Bug 47278 - Add WebThemeEngine api for chromium/linux
Summary: Add WebThemeEngine api for chromium/linux
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-06 10:36 PDT by Dave Moore
Modified: 2010-10-07 08:08 PDT (History)
4 users (show)

See Also:


Attachments
Patch (8.53 KB, patch)
2010-10-06 10:48 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Patch (8.55 KB, patch)
2010-10-06 11:06 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Patch (9.51 KB, patch)
2010-10-06 11:20 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Patch (10.25 KB, patch)
2010-10-06 11:36 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Patch (10.26 KB, patch)
2010-10-06 11:44 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Patch (10.26 KB, patch)
2010-10-06 17:02 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Patch (10.25 KB, patch)
2010-10-07 07:46 PDT, Dave Moore
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Moore 2010-10-06 10:36:26 PDT
Currently all scrollbar drawing for chromium / linux is done within platform/chromium/ScrollbarThemeChromiumLinux.cpp. This prevents us from customizing it from the chromium project. We need to add new api to support this.
Comment 1 Dave Moore 2010-10-06 10:48:43 PDT
Created attachment 69958 [details]
Patch
Comment 2 WebKit Review Bot 2010-10-06 10:55:53 PDT
Attachment 69958 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Dave Moore 2010-10-06 11:06:59 PDT
Created attachment 69960 [details]
Patch
Comment 4 Tony Chang 2010-10-06 11:12:50 PDT
Comment on attachment 69960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69960&action=review

Can you also update WebKit.gyp to include the new files (maybe remove the old file?) and update WebKit/chromium/src/ChromiumBridge.cpp to include win/WebThemeEngine.h?

> ChangeLog:11
> +        * ../../WebKit/chromium/public/WebThemeEngine.h:

This path is incorrect.  Maybe webkit-patch is confused about being an svn checkout in a chromium git checkout?  This path should be relative to the ChangeLog file.
Comment 5 Dave Moore 2010-10-06 11:20:42 PDT
Created attachment 69967 [details]
Patch
Comment 6 Dave Moore 2010-10-06 11:21:53 PDT
Comment on attachment 69967 [details]
Patch

I can't remove the old WebThemeEngine.h without breaking the chromium build. I'll do it once I've added support for the new api there, and changed its includes.
Comment 7 Tony Chang 2010-10-06 11:26:55 PDT
Comment on attachment 69967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69967&action=review

> I can't remove the old WebThemeEngine.h without breaking the chromium build. I'll do it once I've added support for the new api there, and changed its includes.

Right, I just mean to remove it from the gyp file (I think that's harmless, it just changes what file the IDE tries to open).  You can still change the include in WebKit/chromium/src/ChromiumBridge.cpp, right?

r- for ChangeLog

> ChangeLog:1
> +2010-10-06  Dave Moore  <davemoore@chromium.org>

Actually, this is the wrong ChangeLog file.  You want to edit WebKit/ChangeLog not the root ChangeLog.  I think if you're going to use webkit-patch, you need to have a full WebKit checkout.  Alternately, you can probably use the svn based scripts from your WebKit/WebKit/ directory (i.e., manually run prepare-ChangeLog and svn-create-diff).
Comment 8 Dave Moore 2010-10-06 11:36:29 PDT
Created attachment 69969 [details]
Patch
Comment 9 WebKit Review Bot 2010-10-06 11:42:37 PDT
Attachment 69969 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Dave Moore 2010-10-06 11:44:34 PDT
Created attachment 69971 [details]
Patch
Comment 11 Tony Chang 2010-10-06 12:08:50 PDT
Comment on attachment 69971 [details]
Patch

LGTM.  Darin, can you do a quick review too?  (Not setting review flag so Darin can review and so cr-linux can run.)
Comment 12 Darin Fisher (:fishd, Google) 2010-10-06 16:17:23 PDT
Comment on attachment 69971 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69971&action=review

> WebKit/chromium/public/WebThemeEngine.h:42
> +// This file has been moved to the win subdirectory as it's entirely windows

nit: please prefix this with "FIXME:" for easy grepping later

> WebKit/chromium/public/linux/WebThemeEngine.h:67
> +        // The bounds of the entire track, as opposed to the part being painted.

can you use WebRect here?

   WebRect trackBounds
Comment 13 Dave Moore 2010-10-06 17:02:21 PDT
Created attachment 70013 [details]
Patch
Comment 14 Kent Tamura 2010-10-06 17:09:02 PDT
Comment on attachment 70013 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70013&action=review

> ChangeLog:1
> +2010-10-06  Dave Moore  <davemoore@chromium.org>

The patch still update a wrong ChangeLog.  It should be WebKit/chromium/ChangeLog, not the root ChangeLog.
Comment 15 Dave Moore 2010-10-07 07:46:54 PDT
Created attachment 70090 [details]
Patch
Comment 16 WebKit Commit Bot 2010-10-07 08:08:17 PDT
Comment on attachment 70090 [details]
Patch

Clearing flags on attachment: 70090

Committed r69311: <http://trac.webkit.org/changeset/69311>
Comment 17 WebKit Commit Bot 2010-10-07 08:08:24 PDT
All reviewed patches have been landed.  Closing bug.