Bug 97474 – [Chromium] Remove screen-related functions from PlatformSupport

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mark Pilgrim (Google)]

[Chromium] Remove screen-related functions from PlatformSupport

[Mark Pilgrim (Google)]

Created attachment 165430 [details]
WIP Patch

[Mark Pilgrim (Google)]

Created attachment 165680 [details]
WIP Patch

[Mark Pilgrim (Google)]

Created attachment 165685 [details]
WIP Patch

[Mark Pilgrim (Google)]

Created attachment 165697 [details]
WIP Patch

[Mark Pilgrim (Google)]

Created attachment 165700 [details]
WIP Patch

[Mark Pilgrim (Google)]

Created attachment 165826 [details]
WIP Patch

[Mark Pilgrim (Google)]

Created attachment 166059 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

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 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)]

Created attachment 166087 [details]
Patch

[Mark Pilgrim (Google)]

Comment on attachment 166087 [details]
Patch

Now with guards against nonexistent objects in the screenInfo() call chain.

[Adam Barth]

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)]

(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 on attachment 166087 [details]
Patch

I checked hostWindow().  It can definitely be 0.

[WebKit Review Bot]

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]

I'm not sure why the bot is having trouble today.  :(

[WebKit Review Bot]

Comment on attachment 166087 [details]
Patch

Clearing flags on attachment: 166087

Committed r129825: <http://trac.webkit.org/changeset/129825>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

Reverted r129825 for reason:

DOMWindow.resizeTo() is broken. Asked by Mark Pilgrim.

Committed r129894: <http://trac.webkit.org/changeset/129894>

[Mark Pilgrim (Google)]

Created attachment 166542 [details]
Patch

[Mark Pilgrim (Google)]

Comment on attachment 166542 [details]
Patch

Includes patch from bug 97909, also implementing platformPageClient() on PagePopupChromeClient.

[Adam Barth]

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)]

Looks like this one is ready to go?  With a reply from Mark and a review from Adam...

[Mark Pilgrim (Google)]

(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)]

FYI, the latest patch still applies cleanly to ToT.

[Adam Barth]

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)]

Created attachment 170469 [details]
Patch

[Mark Pilgrim (Google)]

Comment on attachment 170469 [details]
Patch

Nits addressed.

[WebKit Review Bot]

Comment on attachment 170469 [details]
Patch

Clearing flags on attachment: 170469

Committed r132419: <http://trac.webkit.org/changeset/132419>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.