Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2906133004:
JavaScript dialogs cause a page to lose fullscreen. (Closed)
|
Created:
3 years, 7 months ago by Avi (use Gerrit) Modified:
3 years, 6 months ago CC:
chromium-reviews, meacer Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionIf JavaScript shows a dialog, cause the page to lose fullscreen.
BUG=670135, 550017, 726761, 728276
Review-Url: https://codereview.chromium.org/2906133004
Cr-Commit-Position: refs/heads/master@{#478884}
Committed: https://chromium.googlesource.com/chromium/src/+/0720b02e4f303ea6b114d4ae9453e3a7ff55f8dc
Patch Set 1 #Patch Set 2 : beforeunload too #
Total comments: 2
Patch Set 3 : rev #Patch Set 4 : with a test #
Total comments: 15
Patch Set 5 : matt #
Messages
Total messages: 38 (29 generated)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
meacer@chromium.org changed reviewers: + meacer@chromium.org
https://codereview.chromium.org/2906133004/diff/20001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc (right): https://codereview.chromium.org/2906133004/diff/20001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:139: parent_web_contents); Drive by: Does this exit window-fullscreen (as opposed to tab-fullscreen)? If so, it might be problematic on Mac where maximizing Chrome causes it to enter window-fullscreen mode (entered with F11 on other platforms)
https://codereview.chromium.org/2906133004/diff/20001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc (right): https://codereview.chromium.org/2906133004/diff/20001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:139: parent_web_contents); On 2017/06/01 22:26:01, meacer wrote: > Drive by: Does this exit window-fullscreen (as opposed to tab-fullscreen)? If > so, it might be problematic on Mac where maximizing Chrome causes it to enter > window-fullscreen mode (entered with F11 on other platforms) No, this is only exiting html5 fullscreen, which is the kind that the page triggers itself. User-initiated fullscreen is completely different than this. (The tangled nature of the fullscreen APIs makes me :( )
Description was changed from ========== JavaScript dialogs cause a page to lose fullscreen. BUG=670135, 550017, 726761 ========== to ========== If JavaScript shows a dialog, cause the page to lose fullscreen. BUG=670135, 550017, 726761, 728276 ==========
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
avi@chromium.org changed reviewers: + mgiuca@chromium.org, thestig@chromium.org - meacer@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Matt, fullscreen. Lei, printing.
Printing bits lgtm.
https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4431: ExitFullscreen(false); I think this should be true... documentation says "|will_cause_resize| indicates whether the fullscreen change causes a view resize. e.g. This will be false when going from tab fullscreen to browser fullscreen." Seems like it should be true in this case. https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4468: ExitFullscreen(false); Same as above. https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:710: // bool IsFullscreenForCurrentTab() const override; Shouldn't have commented-out lines of code. How about just "// IsFullscreenForCurrentTab is already ...." https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1418: IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, Excellent tests, thanks. https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1454: // Disable the hang monitor, otherwise there will be a race between the Nit: Replace the commas with parens: "(otherwise ... timer)". https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1460: script = "window.onbeforeunload=function(e){return 'x'};"; nit: Insert spaces in the JavaScript for readability (shouldn't overflow the line). https://codereview.chromium.org/2906133004/diff/60001/content/public/browser/... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2906133004/diff/60001/content/public/browser/... content/public/browser/web_contents.h:743: virtual bool IsFullscreenForCurrentTab() const = 0; Hmmm... It's a bit weird to have the same method in both base classes of WebContentsImpl. Is there a precedent for this? I guess it's OK.
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
In case I can catch you, Matt, before I disappear for a week. https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4431: ExitFullscreen(false); On 2017/06/09 06:16:37, Matt Giuca wrote: > I think this should be true... documentation says "|will_cause_resize| indicates > whether the fullscreen change causes a view resize. e.g. This will be false when > going from tab fullscreen to browser fullscreen." > > Seems like it should be true in this case. Done. https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4468: ExitFullscreen(false); On 2017/06/09 06:16:37, Matt Giuca wrote: > Same as above. Done. https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:710: // bool IsFullscreenForCurrentTab() const override; On 2017/06/09 06:16:37, Matt Giuca wrote: > Shouldn't have commented-out lines of code. > > How about just "// IsFullscreenForCurrentTab is already ...." This is paralleling lines 689, etc, above. https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1418: IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, On 2017/06/09 06:16:37, Matt Giuca wrote: > Excellent tests, thanks. Acknowledged. https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1454: // Disable the hang monitor, otherwise there will be a race between the On 2017/06/09 06:16:37, Matt Giuca wrote: > Nit: Replace the commas with parens: "(otherwise ... timer)". Done. https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1460: script = "window.onbeforeunload=function(e){return 'x'};"; On 2017/06/09 06:16:37, Matt Giuca wrote: > nit: Insert spaces in the JavaScript for readability (shouldn't overflow the > line). Done. https://codereview.chromium.org/2906133004/diff/60001/content/public/browser/... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2906133004/diff/60001/content/public/browser/... content/public/browser/web_contents.h:743: virtual bool IsFullscreenForCurrentTab() const = 0; On 2017/06/09 06:16:37, Matt Giuca wrote: > Hmmm... It's a bit weird to have the same method in both base classes of > WebContentsImpl. Is there a precedent for this? I guess it's OK. Yes, this is done in a few places.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mgiuca@chromium.org
lgtm Since you're away, I'm gonna be bold and land this for you. Hope you're OK with that. https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/2906133004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:1460: script = "window.onbeforeunload=function(e){return 'x'};"; On 2017/06/09 14:36:23, Avi (OOO Mon 12-Fri 16) wrote: > On 2017/06/09 06:16:37, Matt Giuca wrote: > > nit: Insert spaces in the JavaScript for readability (shouldn't overflow the > > line). > > Done. I meant also around the equals sign. And may as well also have a semicolon. Oh well, let's not block landing this but if you work on this code again maybe do that.
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2906133004/#ps80001 (title: "matt")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497318818413020,
"parent_rev": "3adbcbaa43c1d5069da92d154a7382943c5a2881", "commit_rev":
"0720b02e4f303ea6b114d4ae9453e3a7ff55f8dc"}
Message was sent while issue was closed.
Description was changed from ========== If JavaScript shows a dialog, cause the page to lose fullscreen. BUG=670135, 550017, 726761, 728276 ========== to ========== If JavaScript shows a dialog, cause the page to lose fullscreen. BUG=670135, 550017, 726761, 728276 Review-Url: https://codereview.chromium.org/2906133004 Cr-Commit-Position: refs/heads/master@{#478884} Committed: https://chromium.googlesource.com/chromium/src/+/0720b02e4f303ea6b114d4ae9453... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/0720b02e4f303ea6b114d4ae9453... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
