Bug 97474

Summary: [Chromium] Remove screen-related functions from PlatformSupport
Product: WebKit Reporter: Mark Pilgrim (Google) <pilgrim>
Component: New BugsAssignee: Mark Pilgrim (Google) <pilgrim>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, fishd, haraken, jamesr, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 82948    
Attachments:
Description Flags
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Mark Pilgrim (Google)
Reported 2012-09-24 12:30:38 PDT
[Chromium] Remove screen-related functions from PlatformSupport
Attachments
WIP Patch (16.60 KB, patch)
2012-09-24 12:30 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
WIP Patch (22.96 KB, patch)
2012-09-25 14:49 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
WIP Patch (22.51 KB, patch)
2012-09-25 15:16 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
WIP Patch (22.43 KB, patch)
2012-09-25 16:15 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
WIP Patch (22.66 KB, patch)
2012-09-25 16:30 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
WIP Patch (22.77 KB, patch)
2012-09-26 10:16 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
Patch (23.09 KB, patch)
2012-09-27 13:59 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
Patch (23.81 KB, patch)
2012-09-27 16:07 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
Patch (24.86 KB, patch)
2012-10-01 14:22 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
Patch (24.80 KB, patch)
2012-10-24 14:15 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.
Mark Pilgrim (Google)
Comment 1 2012-09-24 12:30:57 PDT
Created attachment 165430 [details] WIP Patch
Mark Pilgrim (Google)
Comment 2 2012-09-25 14:49:36 PDT
Created attachment 165680 [details] WIP Patch
Mark Pilgrim (Google)
Comment 3 2012-09-25 15:16:37 PDT
Created attachment 165685 [details] WIP Patch
Mark Pilgrim (Google)
Comment 4 2012-09-25 16:15:30 PDT
Created attachment 165697 [details] WIP Patch
Mark Pilgrim (Google)
Comment 5 2012-09-25 16:30:46 PDT
Created attachment 165700 [details] WIP Patch
Mark Pilgrim (Google)
Comment 6 2012-09-26 10:16:06 PDT
Created attachment 165826 [details] WIP Patch
Mark Pilgrim (Google)
Comment 7 2012-09-27 13:59:07 PDT
Created attachment 166059 [details] Patch
WebKit Review Bot
Comment 8 2012-09-27 14:02:38 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Adam Barth
Comment 9 2012-09-27 14:23:36 PDT
Comment on attachment 166059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166059&action=review Very nice. > Source/Platform/chromium/public/WebScreenInfo.h:77 > + , isMonochrome(false) { } Technically the { and } should each be on their own line. > Source/WebKit/chromium/src/ChromeClientImpl.cpp:911 > + You've got an extra blank line here
WebKit Review Bot
Comment 10 2012-09-27 15:30:14 PDT
Comment on attachment 166059 [details] Patch Rejecting attachment 166059 [details] from commit-queue. New failing tests: fast/media/media-svg-crash.html fast/frames/iframe-display-none.html fast/frames/crash-removed-iframe.html Full output: http://queues.webkit.org/results/14061015
Mark Pilgrim (Google)
Comment 11 2012-09-27 16:07:11 PDT
Created attachment 166087 [details] Patch
Mark Pilgrim (Google)
Comment 12 2012-09-27 16:08:11 PDT
Comment on attachment 166087 [details] Patch Now with guards against nonexistent objects in the screenInfo() call chain.
Adam Barth
Comment 13 2012-09-27 16:09:44 PDT
Comment on attachment 166087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166087&action=review > Source/WebCore/platform/chromium/PlatformScreenChromium.cpp:53 > + if (!widget) > + return 0; > + ScrollView* root = widget->root(); > + if (!root) > + return 0; > + HostWindow* hostWindow = root->hostWindow(); > + if (!hostWindow) > + return 0; > + return hostWindow->platformPageClient(); Can all these things really be 0?
Mark Pilgrim (Google)
Comment 14 2012-09-27 16:28:25 PDT
(In reply to comment #13) > (From update of attachment 166087 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166087&action=review > > Can all these things really be 0? root() was in several tests. Don't know about the others. The old code had tons of guards like this in the old PlatformSupport::toWebWidget() function, at every step of the call chain.
Adam Barth
Comment 15 2012-09-27 16:46:36 PDT
Comment on attachment 166087 [details] Patch I checked hostWindow(). It can definitely be 0.
WebKit Review Bot
Comment 16 2012-09-27 16:50:18 PDT
Comment on attachment 166087 [details] Patch Rejecting attachment 166087 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/mac/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/14054099
Adam Barth
Comment 17 2012-09-27 17:00:50 PDT
I'm not sure why the bot is having trouble today. :(
WebKit Review Bot
Comment 18 2012-09-27 17:30:57 PDT
Comment on attachment 166087 [details] Patch Clearing flags on attachment: 166087 Committed r129825: <http://trac.webkit.org/changeset/129825>
WebKit Review Bot
Comment 19 2012-09-27 17:31:01 PDT
All reviewed patches have been landed. Closing bug.
Kentaro Hara
Comment 20 2012-09-28 07:16:04 PDT
Reverted r129825 for reason: DOMWindow.resizeTo() is broken. Asked by Mark Pilgrim. Committed r129894: <http://trac.webkit.org/changeset/129894>
Mark Pilgrim (Google)
Comment 21 2012-10-01 14:22:01 PDT
Created attachment 166542 [details] Patch
Mark Pilgrim (Google)
Comment 22 2012-10-01 14:24:09 PDT
Comment on attachment 166542 [details] Patch Includes patch from bug 97909, also implementing platformPageClient() on PagePopupChromeClient.
Adam Barth
Comment 23 2012-10-01 14:42:50 PDT
What's different in this patch from the previous patch? Should we include a test so we don't make the same mistake again?
Eric Seidel (no email)
Comment 24 2012-10-24 12:49:26 PDT
Looks like this one is ready to go? With a reply from Mark and a review from Adam...
Mark Pilgrim (Google)
Comment 25 2012-10-24 12:51:37 PDT
(In reply to comment #23) > What's different in this patch from the previous patch? Should we include a test so we don't make the same mistake again? What's different is that it includes the patch from bug 97909. Not sure how to make a test for it. Perhaps we could ask Keishi?
Mark Pilgrim (Google)
Comment 26 2012-10-24 13:01:41 PDT
FYI, the latest patch still applies cleanly to ToT.
Adam Barth
Comment 27 2012-10-24 13:58:55 PDT
Comment on attachment 166542 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166542&action=review > Source/WebCore/platform/chromium/PlatformScreenChromium.cpp:92 > + return 0; return false? > Source/WebCore/platform/chromium/PlatformScreenChromium.cpp:100 > + return IntRect(); return FloatRect() ? > Source/WebCore/platform/chromium/PlatformScreenChromium.cpp:108 > + return IntRect(); Return FloatRect() ? > Source/WebKit/chromium/src/ChromeClientImpl.cpp:910 > + You've got an extra blank line here. > Source/WebKit/chromium/src/PlatformSupport.cpp:-369 > - return 0; I see. This goofiness was here already. We should still probably clean it up.
Mark Pilgrim (Google)
Comment 28 2012-10-24 14:15:49 PDT
Created attachment 170469 [details] Patch
Mark Pilgrim (Google)
Comment 29 2012-10-24 14:16:42 PDT
Comment on attachment 170469 [details] Patch Nits addressed.
WebKit Review Bot
Comment 30 2012-10-24 16:20:00 PDT
Comment on attachment 170469 [details] Patch Clearing flags on attachment: 170469 Committed r132419: <http://trac.webkit.org/changeset/132419>
WebKit Review Bot
Comment 31 2012-10-24 16:20:06 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.