Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
903683005:
Always prompt for permission on fullscreen and mouse lock on file:// URLs (Closed)
DescriptionBefore this change, fullscreen and mouse lock were allowed on file:// URLs without giving the user a chance to Allow or Deny them and without showing any persistent UI. This change makes it so that users are always prompted with persistent UI on file:// URLs. There is no way to remember the preference to show or hide the persistent permission bubble because file:// URLs lack a clear origin concept.
BUG=455953, 455882
TEST=Download http://adrifelt.github.io/demos/all-permissions (right click, Save As). Open the downloaded file in Chrome. Click "Fullscreen": a permissions bubble should have "Dismiss" and "Exit full screen" buttons, where "Dismiss" just dismisses the bubble. If you click Dismiss, the bubble should again show up when you enter fullscreen a second time. For Pointer Lock, the bubble should show Allow and Deny buttons, and the mouse should not be locked until you click Allow. Clicking Allow or Deny should not affect the behavior of the bubble if you press Pointer Lock again.
Committed: https://crrev.com/6eca4fd0c63d0b31729e35ad883fb07b27dfca73
Cr-Commit-Position: refs/heads/master@{#316322}
Patch Set 1 #Patch Set 2 : removed accidentally committed unnecessary include #
Total comments: 4
Patch Set 3 : Always ask before going fullscreen or pointer lock on file:// #Patch Set 4 : style fixes #Patch Set 5 : add a comment #Patch Set 6 : add unit test for permissions menu for fullscreen/mouse lock on file:// #
Total comments: 9
Patch Set 7 : Add comments #Patch Set 8 : Updated Mac UI for fullscreen on file:// URLs #
Total comments: 17
Patch Set 9 : style fixes and remove unused includes #
Total comments: 4
Patch Set 10 : tweak unit test #Messages
Total messages: 47 (10 generated)
estark@chromium.org changed reviewers: + lgarron@chromium.org, meacer@chromium.org
This CL disables fullscreen and pointer lock for file:// URLs, instead of just allowing them outright (without asking). The alternative to disabling would be to ask for Allow/Deny and remember the decision for the current URL (not for the origin -- blocking fullscreen for one file:// URL probably shouldn't block it for all file:// URLs). I have an alternative CL that does just that (https://codereview.chromium.org/915473002/, though it's not quite complete), but I eventually decided that it was too big and messy a change for a very nebulous gain. (What does it mean for a user to block permissions just for file:///foo.html if file:///foo.html can open a popup to file:///bar.html and script it however it wants, including full-screening or mouse-locking it?) So I think I convinced myself that the right thing to do is disable fullscreen and pointer lock for now, and re-enable them when file:// URLs are unique origins (crbug.com/455882). Very open to being convinced otherwise though!
scheib@chromium.org changed reviewers: + scheib@chromium.org
Disabling the feature entirely seems overkill. I don't have access the the bug and can't see context. However it seems appropriate to always treat the FILE:// URLS as a ContentSetting of ASK and when the user presses "Allow" simply discard. This would enable the features but prompt users every time.
On 2015/02/10 01:20:01, scheib wrote: > Disabling the feature entirely seems overkill. I don't have access the the bug > and can't see context. However it seems appropriate to always treat the FILE:// > URLS as a ContentSetting of ASK and when the user presses "Allow" simply > discard. This would enable the features but prompt users every time. Fullscreen and pointerlock bubbles essentially exist so that the user can disable the bubble itself. The page is already fullscreen when the bubble shows up. So treating them as always ASK is not meaningful: if the user clicks "Allow", the bubble will still be shown next time, making the bubble pointless.
On 2015/02/10 01:26:03, Mustafa Emre Acer wrote: > On 2015/02/10 01:20:01, scheib wrote: > > Disabling the feature entirely seems overkill. I don't have access the the bug > > and can't see context. However it seems appropriate to always treat the > FILE:// > > URLS as a ContentSetting of ASK and when the user presses "Allow" simply > > discard. This would enable the features but prompt users every time. > > Fullscreen and pointerlock bubbles essentially exist so that the user can > disable the bubble itself. The page is already fullscreen when the bubble shows > up. So treating them as always ASK is not meaningful: if the user clicks > "Allow", the bubble will still be shown next time, making the bubble pointless. Moreover it seems very confusing to give users a Deny button that does something useful on normal pages (blocks future fullscreen requests) but does nothing on file:// URLs. meacer@, do you have any interest in un-restricting the bug?
I've CC'ed scheib@ on the bug so that he can see it. Feel free to drop the view restriction as well, I don't think there is much risk in doing that. https://codereview.chromium.org/903683005/diff/20001/chrome/browser/ui/exclus... File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right): https://codereview.chromium.org/903683005/diff/20001/chrome/browser/ui/exclus... chrome/browser/ui/exclusive_access/fullscreen_controller.cc:115: // fixed. "TODO(estark): Revisit this when crbug.com/455882 is fixed" sounds a bit more formal :) https://codereview.chromium.org/903683005/diff/20001/chrome/browser/ui/exclus... chrome/browser/ui/exclusive_access/fullscreen_controller.cc:118: } Perhaps you could add a browsertest or layout test so that this doesn't regress. For browsertest you could use fullscreen_controller_browsertest.cc. For layout test you could use one of the test cases under third_party/WebKit/LayoutTests/fullscreen/ and use setBaseURL() to change the document's url to a file url. https://codereview.chromium.org/903683005/diff/20001/chrome/browser/ui/exclus... File chrome/browser/ui/exclusive_access/mouse_lock_controller.cc (right): https://codereview.chromium.org/903683005/diff/20001/chrome/browser/ui/exclus... chrome/browser/ui/exclusive_access/mouse_lock_controller.cc:232: return CONTENT_SETTING_BLOCK; Blocking this outright makes the fullscreen and pointer lock permissions in the page info bubble moot for file:// urls, but I guess that's OK for now.
On 2015/02/10 01:29:21, emily wrote: > On 2015/02/10 01:26:03, Mustafa Emre Acer wrote: > > On 2015/02/10 01:20:01, scheib wrote: > > > Disabling the feature entirely seems overkill. I don't have access the the > bug > > > and can't see context. However it seems appropriate to always treat the > > FILE:// > > > URLS as a ContentSetting of ASK and when the user presses "Allow" simply > > > discard. This would enable the features but prompt users every time. > > > > Fullscreen and pointerlock bubbles essentially exist so that the user can > > disable the bubble itself. The page is already fullscreen when the bubble > shows > > up. So treating them as always ASK is not meaningful: if the user clicks > > "Allow", the bubble will still be shown next time, making the bubble > pointless. > > Moreover it seems very confusing to give users a Deny button that does something > useful on normal pages (blocks future fullscreen requests) but does nothing on > file:// URLs. > > meacer@, do you have any interest in un-restricting the bug? Fullscreen: The bubble prevents tricking users by displaying false OS and browser UI and leading them to type information intended for another origin. The persistent bubble prevents this UI spoofing by staying until the user dismisses it. This is still valid for file URLs. I believe the correct fix is to cause the bubble to remain visible until the user presses Allow. Deny should exit fullscreen. Pointer Lock: On http(s) schemes Pointer Lock is not entered until Allow is pressed. I'm not 100% on what I think is best, allowing pointer lock for all file schemes or requiring a prompt every time. However, pointer lock is not a security nor privacy threat. It could be a nuisance, but I don't think that justifies disabling the feature. I suggest adjusting this patch to treat every fullscreen request as an ASK and removing changes for pointer lock.
On 2015/02/10 04:03:14, scheib wrote: > On 2015/02/10 01:29:21, emily wrote: > > On 2015/02/10 01:26:03, Mustafa Emre Acer wrote: > > > On 2015/02/10 01:20:01, scheib wrote: > > > > Disabling the feature entirely seems overkill. I don't have access the the > > bug > > > > and can't see context. However it seems appropriate to always treat the > > > FILE:// > > > > URLS as a ContentSetting of ASK and when the user presses "Allow" simply > > > > discard. This would enable the features but prompt users every time. > > > > > > Fullscreen and pointerlock bubbles essentially exist so that the user can > > > disable the bubble itself. The page is already fullscreen when the bubble > > shows > > > up. So treating them as always ASK is not meaningful: if the user clicks > > > "Allow", the bubble will still be shown next time, making the bubble > > pointless. > > > > Moreover it seems very confusing to give users a Deny button that does > something > > useful on normal pages (blocks future fullscreen requests) but does nothing on > > file:// URLs. > > > > meacer@, do you have any interest in un-restricting the bug? > > Fullscreen: The bubble prevents tricking users by displaying false OS and > browser UI and leading them to type information intended for another origin. The > persistent bubble prevents this UI spoofing by staying until the user dismisses > it. This is still valid for file URLs. I believe the correct fix is to cause the > bubble to remain visible until the user presses Allow. Deny should exit > fullscreen. Hmm, I'm not so sure for file:// URLs. There's no meaningful origin to display, so the bubble just says "This page is now full screen." The user can't really make a meaningful decision from that. > > Pointer Lock: On http(s) schemes Pointer Lock is not entered until Allow is > pressed. I'm not 100% on what I think is best, allowing pointer lock for all > file schemes or requiring a prompt every time. However, pointer lock is not a > security nor privacy threat. It could be a nuisance, but I don't think that > justifies disabling the feature. > > I suggest adjusting this patch to treat every fullscreen request as an ASK and > removing changes for pointer lock.
On 2015/02/10 04:03:14, scheib wrote: > On 2015/02/10 01:29:21, emily wrote: > > On 2015/02/10 01:26:03, Mustafa Emre Acer wrote: > > > On 2015/02/10 01:20:01, scheib wrote: > > > > Disabling the feature entirely seems overkill. I don't have access the the > > bug > > > > and can't see context. However it seems appropriate to always treat the > > > FILE:// > > > > URLS as a ContentSetting of ASK and when the user presses "Allow" simply > > > > discard. This would enable the features but prompt users every time. > > > > > > Fullscreen and pointerlock bubbles essentially exist so that the user can > > > disable the bubble itself. The page is already fullscreen when the bubble > > shows > > > up. So treating them as always ASK is not meaningful: if the user clicks > > > "Allow", the bubble will still be shown next time, making the bubble > > pointless. > > > > Moreover it seems very confusing to give users a Deny button that does > something > > useful on normal pages (blocks future fullscreen requests) but does nothing on > > file:// URLs. > > > > meacer@, do you have any interest in un-restricting the bug? > > Fullscreen: The bubble prevents tricking users by displaying false OS and > browser UI and leading them to type information intended for another origin. The > persistent bubble prevents this UI spoofing by staying until the user dismisses > it. This is still valid for file URLs. I believe the correct fix is to cause the > bubble to remain visible until the user presses Allow. Deny should exit > fullscreen. > > Pointer Lock: On http(s) schemes Pointer Lock is not entered until Allow is > pressed. I'm not 100% on what I think is best, allowing pointer lock for all > file schemes or requiring a prompt every time. However, pointer lock is not a > security nor privacy threat. It could be a nuisance, but I don't think that > justifies disabling the feature. > > I suggest adjusting this patch to treat every fullscreen request as an ASK and > removing changes for pointer lock. As mentioned above, there is no point in having an "Allow" button if we are going to show the bubble every time. "Allow" has a different meaning for fullscreen. The page is already fullscreen when the bubble shows, "Allow" merely dismisses the bubble from further invocations of requestFullscreen. Having it there while asking every time just doesn't make sense. If we are going to ask everytime, we should change the label to something else like "Dismiss", or just remove "Allow" altogether. Having said that, what is the use case for a file:// page to enter fullscreen mode or get pointer lock? Other APIs (notification, audio, video and location) just don't work on file:// urls, why should fullscreen do?
On 2015/02/10 at 04:55:44, meacer wrote: > On 2015/02/10 04:03:14, scheib wrote: > > On 2015/02/10 01:29:21, emily wrote: > > > On 2015/02/10 01:26:03, Mustafa Emre Acer wrote: > > > > On 2015/02/10 01:20:01, scheib wrote: > > > > > Disabling the feature entirely seems overkill. I don't have access the the > > > bug > > > > > and can't see context. However it seems appropriate to always treat the > > > > FILE:// > > > > > URLS as a ContentSetting of ASK and when the user presses "Allow" simply > > > > > discard. This would enable the features but prompt users every time. > > > > > > > > Fullscreen and pointerlock bubbles essentially exist so that the user can > > > > disable the bubble itself. The page is already fullscreen when the bubble > > > shows > > > > up. So treating them as always ASK is not meaningful: if the user clicks > > > > "Allow", the bubble will still be shown next time, making the bubble > > > pointless. > > > > > > Moreover it seems very confusing to give users a Deny button that does > > something > > > useful on normal pages (blocks future fullscreen requests) but does nothing on > > > file:// URLs. > > > > > > meacer@, do you have any interest in un-restricting the bug? > > > > Fullscreen: The bubble prevents tricking users by displaying false OS and > > browser UI and leading them to type information intended for another origin. The > > persistent bubble prevents this UI spoofing by staying until the user dismisses > > it. This is still valid for file URLs. I believe the correct fix is to cause the > > bubble to remain visible until the user presses Allow. Deny should exit > > fullscreen. > > > > > Pointer Lock: On http(s) schemes Pointer Lock is not entered until Allow is > > pressed. I'm not 100% on what I think is best, allowing pointer lock for all > > file schemes or requiring a prompt every time. However, pointer lock is not a > > security nor privacy threat. It could be a nuisance, but I don't think that > > justifies disabling the feature. > > > > I suggest adjusting this patch to treat every fullscreen request as an ASK and > > removing changes for pointer lock. > > As mentioned above, there is no point in having an "Allow" button if we are going to show the bubble every time. "Allow" has a different meaning for fullscreen. The page is already fullscreen when the bubble shows, "Allow" merely dismisses the bubble from further invocations of requestFullscreen. Having it there while asking every time just doesn't make sense. > > If we are going to ask everytime, we should change the label to something else like "Dismiss", or just remove "Allow" altogether. > > Having said that, what is the use case for a file:// page to enter fullscreen mode or get pointer lock? Other APIs (notification, audio, video and location) just don't work on file:// urls, why should fullscreen do? I haven't had time to read the rest of this CR yet, but I'm always a little disappointed that we're taking away power from tinkerable "local" pages when there is no equally democratic alternative. (Other solutions involve browser extensions and local servers, which require non-trivial knowledge and might be restricted on some devices.) I believe fullscreen should be possible because Javascript on a page can ask for fullscreen – unless there's a good security/UX reason to treat them differently.
On 2015/02/10 06:10:01, lgarron wrote: > On 2015/02/10 at 04:55:44, meacer wrote: > > On 2015/02/10 04:03:14, scheib wrote: > > > On 2015/02/10 01:29:21, emily wrote: > > > > On 2015/02/10 01:26:03, Mustafa Emre Acer wrote: > > > > > On 2015/02/10 01:20:01, scheib wrote: > > > > > > Disabling the feature entirely seems overkill. I don't have access the > the > > > > bug > > > > > > and can't see context. However it seems appropriate to always treat > the > > > > > FILE:// > > > > > > URLS as a ContentSetting of ASK and when the user presses "Allow" > simply > > > > > > discard. This would enable the features but prompt users every time. > > > > > > > > > > Fullscreen and pointerlock bubbles essentially exist so that the user > can > > > > > disable the bubble itself. The page is already fullscreen when the > bubble > > > > shows > > > > > up. So treating them as always ASK is not meaningful: if the user clicks > > > > > "Allow", the bubble will still be shown next time, making the bubble > > > > pointless. > > > > > > > > Moreover it seems very confusing to give users a Deny button that does > > > something > > > > useful on normal pages (blocks future fullscreen requests) but does > nothing on > > > > file:// URLs. > > > > > > > > meacer@, do you have any interest in un-restricting the bug? > > > > > > Fullscreen: The bubble prevents tricking users by displaying false OS and > > > browser UI and leading them to type information intended for another origin. > The > > > persistent bubble prevents this UI spoofing by staying until the user > dismisses > > > it. This is still valid for file URLs. I believe the correct fix is to cause > the > > > bubble to remain visible until the user presses Allow. Deny should exit > > > fullscreen. > > > > > > > > Pointer Lock: On http(s) schemes Pointer Lock is not entered until Allow is > > > pressed. I'm not 100% on what I think is best, allowing pointer lock for all > > > file schemes or requiring a prompt every time. However, pointer lock is not > a > > > security nor privacy threat. It could be a nuisance, but I don't think that > > > justifies disabling the feature. > > > > > > I suggest adjusting this patch to treat every fullscreen request as an ASK > and > > > removing changes for pointer lock. > > > > As mentioned above, there is no point in having an "Allow" button if we are > going to show the bubble every time. "Allow" has a different meaning for > fullscreen. The page is already fullscreen when the bubble shows, "Allow" merely > dismisses the bubble from further invocations of requestFullscreen. Having it > there while asking every time just doesn't make sense. > > > > If we are going to ask everytime, we should change the label to something else > like "Dismiss", or just remove "Allow" altogether. > > > > Having said that, what is the use case for a file:// page to enter fullscreen > mode or get pointer lock? Other APIs (notification, audio, video and location) > just don't work on file:// urls, why should fullscreen do? > > I haven't had time to read the rest of this CR yet, but I'm always a little > disappointed that we're taking away power from tinkerable "local" pages when > there is no equally democratic alternative. (Other solutions involve browser > extensions and local servers, which require non-trivial knowledge and might be > restricted on some devices.) > > I believe fullscreen should be possible because Javascript on a page can ask for > fullscreen – unless there's a good security/UX reason to treat them differently. Well, I do think that there might be a security reason to treat file:// URLs differently: there's no user-meaningful origin to show in the bubble. Plus there's no sensible way to let users block fullscreen on file:// URLs, and that doesn't seem right. As long as file:// URLs are all same-origin, we could make it so that blocking fullscreen on file:///foo.html blocks fullscreen on all file:// URLs, but that's annoying; or we could let users block fullscreen on a particular file:// URL, but that would just be downright misleading. Even when file:// URLs are all unique origins, it doesn't make sense (to me) to offer users the ability to block fullscreen on file:///test.html, because a week from now file:///test.html could be a totally different document. In other words, users can't make informed decisions about whether to trust a fullscreened file:// URL (no displayed origin), and we can't give users a reasonable way to deny fullscreen permissions to file:// URLs.
https://codereview.chromium.org/903683005/diff/20001/chrome/browser/ui/exclus... File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right): https://codereview.chromium.org/903683005/diff/20001/chrome/browser/ui/exclus... chrome/browser/ui/exclusive_access/fullscreen_controller.cc:118: } On 2015/02/10 01:47:21, Mustafa Emre Acer wrote: > Perhaps you could add a browsertest or layout test so that this doesn't regress. > For browsertest you could use fullscreen_controller_browsertest.cc. For layout > test you could use one of the test cases under > third_party/WebKit/LayoutTests/fullscreen/ and use setBaseURL() to change the > document's url to a file url. Ah, that was actually a question that I had that I forgotten to ask. I didn't do a browser test initially because it looked like most (all?) of the fullscreen browser tests are disabled due to flakiness. I didn't know that layout tests were an option though -- I'll add one of those (assuming we decide to go forward with this change.)
On 2015/02/10 06:51:30, emily wrote: > On 2015/02/10 06:10:01, lgarron wrote: > > On 2015/02/10 at 04:55:44, meacer wrote: > > > On 2015/02/10 04:03:14, scheib wrote: > > > > On 2015/02/10 01:29:21, emily wrote: > > > > > On 2015/02/10 01:26:03, Mustafa Emre Acer wrote: > > > > > > On 2015/02/10 01:20:01, scheib wrote: > > > > > > > Disabling the feature entirely seems overkill. I don't have access > the > > the > > > > > bug > > > > > > > and can't see context. However it seems appropriate to always treat > > the > > > > > > FILE:// > > > > > > > URLS as a ContentSetting of ASK and when the user presses "Allow" > > simply > > > > > > > discard. This would enable the features but prompt users every time. > > > > > > > > > > > > Fullscreen and pointerlock bubbles essentially exist so that the user > > can > > > > > > disable the bubble itself. The page is already fullscreen when the > > bubble > > > > > shows > > > > > > up. So treating them as always ASK is not meaningful: if the user > clicks > > > > > > "Allow", the bubble will still be shown next time, making the bubble > > > > > pointless. > > > > > > > > > > Moreover it seems very confusing to give users a Deny button that does > > > > something > > > > > useful on normal pages (blocks future fullscreen requests) but does > > nothing on > > > > > file:// URLs. > > > > > > > > > > meacer@, do you have any interest in un-restricting the bug? > > > > > > > > Fullscreen: The bubble prevents tricking users by displaying false OS and > > > > browser UI and leading them to type information intended for another > origin. > > The > > > > persistent bubble prevents this UI spoofing by staying until the user > > dismisses > > > > it. This is still valid for file URLs. I believe the correct fix is to > cause > > the > > > > bubble to remain visible until the user presses Allow. Deny should exit > > > > fullscreen. > > > > > > > > > > > Pointer Lock: On http(s) schemes Pointer Lock is not entered until Allow > is > > > > pressed. I'm not 100% on what I think is best, allowing pointer lock for > all > > > > file schemes or requiring a prompt every time. However, pointer lock is > not > > a > > > > security nor privacy threat. It could be a nuisance, but I don't think > that > > > > justifies disabling the feature. > > > > > > > > I suggest adjusting this patch to treat every fullscreen request as an ASK > > and > > > > removing changes for pointer lock. > > > > > > As mentioned above, there is no point in having an "Allow" button if we are > > going to show the bubble every time. I disagree. Allow permits fullscreen without persistent UI providing a visual indication that the content is fullscreen and offering a mechanism to exit. > > "Allow" has a different meaning for > > fullscreen. The page is already fullscreen when the bubble shows, "Allow" > merely > > dismisses the bubble from further invocations of requestFullscreen. Having it > > there while asking every time just doesn't make sense. > > > > > > If we are going to ask everytime, we should change the label to something > else > > like "Dismiss", or just remove "Allow" altogether. I support changings improving the UI. > > > > > > Having said that, what is the use case for a file:// page to enter > fullscreen > > mode or get pointer lock? The intrinsic functions of fullscreen and pointerlock. > > Other APIs (notification, audio, video and location) > > just don't work on file:// urls, why should fullscreen do? Presumably because of some security or privacy concern. I don't see how they apply to fullscreen or pointer lock. > > > > I haven't had time to read the rest of this CR yet, but I'm always a little > > disappointed that we're taking away power from tinkerable "local" pages when > > there is no equally democratic alternative. (Other solutions involve browser > > extensions and local servers, which require non-trivial knowledge and might be > > restricted on some devices.) > > > > I believe fullscreen should be possible because Javascript on a page can ask > for > > fullscreen – unless there's a good security/UX reason to treat them > differently. > > Well, I do think that there might be a security reason to treat file:// URLs > differently: there's no user-meaningful origin to show in the bubble. The user is making a decision to allow the page to be fullscreen. If the decision is a one-off for just this instance of entering fullscreen then the origin doesn't matter as the decision isn't persisted to an origin. > > Plus there's no sensible way to let users block fullscreen on file:// URLs, and > that doesn't seem right. Given the trade off of no fullscreen for any file:// URL or fullscreen with persistent UI with exit instructions for all file:// URLs I will favor the latter. > As long as file:// URLs are all same-origin, we could > make it so that blocking fullscreen on file:///foo.html blocks fullscreen on all > file:// URLs, but that's annoying; or we could let users block fullscreen on a > particular file:// URL, but that would just be downright misleading. Even when > file:// URLs are all unique origins, it doesn't make sense (to me) to offer > users the ability to block fullscreen on file:///test.html, because a week from > now file:///test.html could be a totally different document. The same is true for https://domain.com/test.html > > In other words, users can't make informed decisions about whether to trust a > fullscreened file:// URL (no displayed origin), and we can't give users a > reasonable way to deny fullscreen permissions to file:// URLs. I want to be clear, the patch as currently formed does not LGTM. We can improve the file:// experience for fullscreen and pointerlock, but I don't see a need to remove the features.
> I disagree. Allow permits fullscreen without persistent UI providing a visual > indication that the content is fullscreen and offering a mechanism to exit. Which is the same thing as dismissing the bubble once and for all, I'm not sure what you are disagreeing with. That said, I'm OK with changing the button to "Dismiss" and keeping it as always ASK for both fullscreen and pointerlock. We need to make sure clicking "Dismiss" doesn't add a fullscreen/pointerlock exception for that page.
Here's a new attempt that always prompts on file:// URLs. It shows a Dismiss button instead of Allow for fullscreen, since the purpose of the button is just to dismiss the bubble. For mouse lock, the bubble always shows Allow and Deny, since in that case the user is choosing to Allow or Deny this specific request for mouse lock. (The alternative is to not request permission at all, but I think that could be dangerous due to security concerns listed in the spec.) In both fullscreen and mouse lock, clicking the bubble buttons does not save any settings; it only affects the individual fullscreen/mouse lock request. I added two browser tests, but they are pretty basic -- they only test that crbug.com/455953 hasn't regressed. I'd appreciate any guidance as to how to test that, for example, clicking Dismiss dismisses the bubble but doesn't save any settings. It looks like all browser tests that click buttons in the bubble have been disabled for flakiness, and I'm not sure if it's possible to click a button in the bubble from a layout test. Finally, I made it so that "Ask" is the only setting that shows up in the origin info bubble for fullscreen and mouse lock on file:// URLs. Also not sure how this CL interacts with crbug.com/352425 that scheib@ pointed out. Maybe the right thing is to do nothing until that issue is resolved?
Great, I'll review soon. On 2015/02/11 00:27:42, emily wrote: > Here's a new attempt that always prompts on file:// URLs. It shows a Dismiss > button instead of Allow for fullscreen, since the purpose of the button is just > to dismiss the bubble. > > For mouse lock, the bubble always shows Allow and Deny, since in that case the > user is choosing to Allow or Deny this specific request for mouse lock. (The > alternative is to not request permission at all, but I think that could be > dangerous due to security concerns listed in the spec.) > > In both fullscreen and mouse lock, clicking the bubble buttons does not save any > settings; it only affects the individual fullscreen/mouse lock request. > > I added two browser tests, but they are pretty basic -- they only test that > crbug.com/455953 hasn't regressed. I'd appreciate any guidance as to how to test > that, for example, clicking Dismiss dismisses the bubble but doesn't save any > settings. It looks like all browser tests that click buttons in the bubble have > been disabled for flakiness, and I'm not sure if it's possible to click a button > in the bubble from a layout test. Fullscreen and pointer lock have a terrible history of not meeting the 99.99% uptime needs we have for automated tests. It's accepted at this point to place detailed TEST= instructions for the changes that QA will review. They already do a manual pass for fullscreen and mouse lock in releases. > > Finally, I made it so that "Ask" is the only setting that shows up in the origin > info bubble for fullscreen and mouse lock on file:// URLs. > > Also not sure how this CL interacts with crbug.com/352425 that scheib@ pointed > out. Maybe the right thing is to do nothing until that issue is resolved?
Thanks Emily, I think this is looking good. I'll defer the review to Vincent since he's the expert on fullscreen and mouselock. https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble_type.cc (right): https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble_type.cc:116: // the user is opting to just Dimiss the dialog rather than Allow Dimiss -> Dismiss https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_menu_model.cc (right): https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_menu_model.cc:51: url.SchemeIsFile(); Looks like you could do an early return from here?
lgtm I suggest adding more comments. Please update the change description as the patch no longer disables the features. Also expand TEST= instructions and reach out to vivianz@chromium.org to add file:// URL testing to the recurring fullscreen and pointer lock test plan. https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right): https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/fullscreen_controller.cc:352: if (!requester.SchemeIsFile() && !embedder.SchemeIsFile() && Add comment along lines of: "Do not store preference on file:// URLs, they don't have a clean origin policy. TODO(estark): Revisit this when crbug.com/455882 is fixed." https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/mouse_lock_controller.cc (right): https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/mouse_lock_controller.cc:146: if (!url.SchemeIsFile() && pattern.IsValid()) { Add comment along lines of: "Do not store preference on file:// URLs, they don't have a clean origin policy. TODO(estark): Revisit this when crbug.com/455882 is fixed."
Oh, and I didn't see any change to Mac -- if you haven't yet you should verify the UI functions appropriately there.
New patchsets have been uploaded after l-g-t-m from scheib@chromium.org
On 2015/02/11 16:48:34, scheib wrote: > lgtm > > I suggest adding more comments. > > Please update the change description as the patch no longer disables the > features. Also expand TEST= instructions and reach out to mailto:vivianz@chromium.org > to add file:// URL testing to the recurring fullscreen and pointer lock test > plan. Done. > > https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... > File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right): > > https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... > chrome/browser/ui/exclusive_access/fullscreen_controller.cc:352: if > (!requester.SchemeIsFile() && !embedder.SchemeIsFile() && > Add comment along lines of: > "Do not store preference on file:// URLs, they don't have a clean origin policy. > TODO(estark): Revisit this when crbug.com/455882 is fixed." > > https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... > File chrome/browser/ui/exclusive_access/mouse_lock_controller.cc (right): > > https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... > chrome/browser/ui/exclusive_access/mouse_lock_controller.cc:146: if > (!url.SchemeIsFile() && pattern.IsValid()) { > Add comment along lines of: > "Do not store preference on file:// URLs, they don't have a clean origin policy. > TODO(estark): Revisit this when crbug.com/455882 is fixed."
https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble_type.cc (right): https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble_type.cc:116: // the user is opting to just Dimiss the dialog rather than Allow On 2015/02/11 00:45:33, Mustafa Emre Acer wrote: > Dimiss -> Dismiss Done. https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right): https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/fullscreen_controller.cc:352: if (!requester.SchemeIsFile() && !embedder.SchemeIsFile() && On 2015/02/11 16:48:34, scheib wrote: > Add comment along lines of: > "Do not store preference on file:// URLs, they don't have a clean origin policy. > TODO(estark): Revisit this when crbug.com/455882 is fixed." Done. https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/mouse_lock_controller.cc (right): https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/mouse_lock_controller.cc:146: if (!url.SchemeIsFile() && pattern.IsValid()) { On 2015/02/11 16:48:34, scheib wrote: > Add comment along lines of: > "Do not store preference on file:// URLs, they don't have a clean origin policy. > TODO(estark): Revisit this when crbug.com/455882 is fixed." Done. https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_menu_model.cc (right): https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_menu_model.cc:51: url.SchemeIsFile(); On 2015/02/11 00:45:33, Mustafa Emre Acer wrote: > Looks like you could do an early return from here? Agree logically, but I'm hesitant to introduce a return point in the middle of the function, out of fear that someone will come along and add completely unrelated code at the bottom of the function without realizing that their code won't run in this special case. What do you think? (I'm not usually as argumentative as I come across in this CL, I swear!)
LGTM too. Thanks for fixing this! https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_menu_model.cc (right): https://codereview.chromium.org/903683005/diff/100001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_menu_model.cc:51: url.SchemeIsFile(); On 2015/02/11 21:21:58, emily wrote: > On 2015/02/11 00:45:33, Mustafa Emre Acer wrote: > > Looks like you could do an early return from here? > > Agree logically, but I'm hesitant to introduce a return point in the middle of > the function, out of fear that someone will come along and add completely > unrelated code at the bottom of the function without realizing that their code > won't run in this special case. What do you think? I'd say that that would in fact be good, they should have added a test case for it :) But it's okay if you want to leave it as it is as well. > > (I'm not usually as argumentative as I come across in this CL, I swear!) But arguing is fun!
lgtm still (hmm, patch after lgtm notices...) And thanks from me as well - good to see improvements here.
New patchsets have been uploaded after l-g-t-m from meacer@chromium.org,scheib@chromium.org
On 2015/02/11 16:49:29, scheib wrote: > Oh, and I didn't see any change to Mac -- if you haven't yet you should verify > the UI functions appropriately there. Done.
lgtm
estark@chromium.org changed reviewers: + groby@chromium.org, markusheintz@chromium.org, msw@chromium.org
groby@: Could you please review changes in chrome/browser/ui/cocoa? markusheintz@: Could you please review changes in chrome/browser/ui/website_settings? msw@: Could you please review changes in chrome/browser/ui/views? Thank you all!
lgtm with nits, and I'm relying on others to review the details of permission_menu_model.cc more closely than I can. https://codereview.chromium.org/903683005/diff/140001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/903683005/diff/140001/chrome/app/generated_re... chrome/app/generated_resources.grd:13647: + <message name="IDS_FULLSCREEN_DISMISS" desc="Text in the bubble button that allows a page to stay in fullscreen without persistent UI warning the user that they are in fullscreen mode."> I don't think "Dismiss" is significantly better than "Allow", but w/e. https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble_type.cc (right): https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble_type.cc:118: if (url.SchemeIsFile()) { nit: remove curly braces. https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble_type.cc:132: default: nit: Remove the default case (and move the NOTREACHED() and return outside the switch) to cause compiler errors if anyone adds ExclusiveAccessBubbleType values without handling them here. Ditto for the 3 switches above. https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right): https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/fullscreen_controller.cc:355: // TODO(estark): Revisit this when crbug.com/455882 is fixed. nit: consider adding this bug to the BUG= in the CL description, just so owners there might be more inclined to go back alert you or revisit this themselves. https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/fullscreen_controller_browsertest.cc (right): https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/fullscreen_controller_browsertest.cc:15: #include "content/public/browser/render_view_host.h" nit: this and some other headers may not be used anymore, I thought we had an iwyu script, but I can't find it, maybe take a quick manual look through these? https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/fullscreen_controller_browsertest.cc:20: using url::kAboutBlankURL; nit: remove this, uses of kAboutBlankURL qualify the url namespace anyway. (optionally remove others by propagating their namespace qualifications too) https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/fullscreen_controller_browsertest.cc:24: class FullscreenControllerBrowserTest: public FullscreenControllerTest { nit: remove this (it's also missing a space before the colon). https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/fullscreen_controller_browsertest.cc:79: IN_PROC_BROWSER_TEST_F(FullscreenControllerTest, MouseLockOnFileURL) { You may want to add a file URL test case to fullscreen_controller_interactive_browsertest.cc, but so many of those tests are DISABLED_ or MAYBE_ for flakiness that maybe that isn't terribly worthwhile... https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_menu_model.cc (right): https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_menu_model.cc:40: break; I hope someone familiar with content permissions reviewed this closely. I idly wonder if it's possible for some Content setting to slip through here without assigning anything to |label|. It looks like CONTENT_SETTING_DEFAULT and CONTENT_SETTING_SESSION_ONLY are not handled in this switch statement, and the new limitations on the if blocks below could theoretically allow some empty labels. (I didn't look too closely...) https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_menu_model_unittest.cc (right): https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_menu_model_unittest.cc:67: EXPECT_EQ(1, fullscreen_model.GetItemCount()); nit: Maybe check GetCommandIdAt(0) or GetLabelAt(0) here and below.
c/b/ui/cocoa LGTM w/nit https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm (right): https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:265: // Update the title of allowButton_ and denyButton_ according to the nit: Mind writing this is |allowButton_| and |denyButton_|? (and |bubbleType_| below?)
New patchsets have been uploaded after l-g-t-m from scheib@chromium.org,msw@chromium.org,groby@chromium.org
https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm (right): https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:265: // Update the title of allowButton_ and denyButton_ according to the On 2015/02/12 00:54:34, groby wrote: > nit: Mind writing this is |allowButton_| and |denyButton_|? (and |bubbleType_| > below?) Done. https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble_type.cc (right): https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble_type.cc:118: if (url.SchemeIsFile()) { On 2015/02/12 00:07:59, msw wrote: > nit: remove curly braces. Done. https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble_type.cc:132: default: On 2015/02/12 00:07:59, msw wrote: > nit: Remove the default case (and move the NOTREACHED() and return outside the > switch) to cause compiler errors if anyone adds ExclusiveAccessBubbleType values > without handling them here. Ditto for the 3 switches above. Done. https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right): https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/fullscreen_controller.cc:355: // TODO(estark): Revisit this when crbug.com/455882 is fixed. On 2015/02/12 00:07:59, msw wrote: > nit: consider adding this bug to the BUG= in the CL description, just so owners > there might be more inclined to go back alert you or revisit this themselves. Done. https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/fullscreen_controller_browsertest.cc (right): https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/fullscreen_controller_browsertest.cc:15: #include "content/public/browser/render_view_host.h" On 2015/02/12 00:07:59, msw wrote: > nit: this and some other headers may not be used anymore, I thought we had an > iwyu script, but I can't find it, maybe take a quick manual look through these? Done. https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_menu_model_unittest.cc (right): https://codereview.chromium.org/903683005/diff/140001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_menu_model_unittest.cc:67: EXPECT_EQ(1, fullscreen_model.GetItemCount()); On 2015/02/12 00:07:59, msw wrote: > nit: Maybe check GetCommandIdAt(0) or GetLabelAt(0) here and below. Done.
estark@chromium.org changed reviewers: - lgarron@chromium.org
https://codereview.chromium.org/903683005/diff/160001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_menu_model_unittest.cc (right): https://codereview.chromium.org/903683005/diff/160001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_menu_model_unittest.cc:5: #include "base/strings/utf_string_conversions.h" nit: is this actually needed? https://codereview.chromium.org/903683005/diff/160001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_menu_model_unittest.cc:70: .compare(fullscreen_model.GetLabelAt(0)) == 0); nit: you should be able to use EXPECT_EQ with these string16s.
https://codereview.chromium.org/903683005/diff/160001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_menu_model_unittest.cc (right): https://codereview.chromium.org/903683005/diff/160001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_menu_model_unittest.cc:5: #include "base/strings/utf_string_conversions.h" On 2015/02/12 21:09:48, msw wrote: > nit: is this actually needed? Oops, no -- removed. https://codereview.chromium.org/903683005/diff/160001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_menu_model_unittest.cc:70: .compare(fullscreen_model.GetLabelAt(0)) == 0); On 2015/02/12 21:09:48, msw wrote: > nit: you should be able to use EXPECT_EQ with these string16s. Done.
lgtm. I hope someone knowledgeable reviewed permission_menu_model.cc
On 2015/02/12 23:07:46, msw wrote: > lgtm. I hope someone knowledgeable reviewed permission_menu_model.cc permissions_menu_mode* LGTM
The CQ bit was checked by estark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/903683005/180001
The CQ bit was unchecked by estark@chromium.org
The CQ bit was checked by estark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/903683005/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/6eca4fd0c63d0b31729e35ad883fb07b27dfca73 Cr-Commit-Position: refs/heads/master@{#316322}
Message was sent while issue was closed.
Woohoo, my very first commit!! Thanks so much for the feedback and help, everyone :) |
