Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2391353003:
Prevent inline install on fullscreen windows. (Closed)
DescriptionPrevent inline install on fullscreen windows.
BUG=488143
Committed: https://crrev.com/6ca20846938a643f34ea9ead734c81c4c6ebd74b
Cr-Commit-Position: refs/heads/master@{#424254}
Patch Set 1 #Patch Set 2 : Prevent install from tab for fullscreen as well and fix browser test. #
Total comments: 14
Patch Set 3 : Updating with reviewer commments. #
Total comments: 1
Messages
Total messages: 19 (5 generated)
Description was changed from ========== Prevent inline install on fullscreen windows. BUG= ========== to ========== Prevent inline install on fullscreen windows. BUG=488143 ==========
ackermanb@chromium.org changed reviewers: + meacer@chromium.org, rdevlin.cronin@chromium.org
Please take a look.
https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/webstore_inline_installer.cc (right): https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer.cc:146: if (controller->IsFullscreenForBrowser() | controller->IsTabFullscreen()) { | is the bitwise operator. You want || instead. https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right): https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:260: BlockInlineInstallFromFullscreenForBrowser) { nit: Can you add a comment for this test explaining how browser fullscreen is different from tab fullscreen? E.g. // Prevent inline install while in browser fullscreen mode. Browser fullscreen // is entered by the user by pressing F11. (and same for the next test case) https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:261: GURL install_url = nit: const GURL install_url = https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:264: AutoAcceptInstall(); You probably don't need this since the dialog won't open in the first place? https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:281: GURL install_url = nit: const GURL install_url = https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:284: AutoAcceptInstall(); Also not needed?
https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right): https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:264: AutoAcceptInstall(); On 2016/10/10 17:57:33, Mustafa Emre Acer wrote: > You probably don't need this since the dialog won't open in the first place? I think I suggested this when Ben was talking to me about it. I wanted to make sure that everything *would* succeed if the error wasn't thrown. Without this check, there could be a silent test failure where the extension doesn't get installed, but the reason it doesn't is because we stall at the dialog showing - when in fact the dialog should never be shown. It's probably a little superfluous with the error checks in the js, but it makes me feel a bit better.
PTAL. https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/webstore_inline_installer.cc (right): https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer.cc:146: if (controller->IsFullscreenForBrowser() | controller->IsTabFullscreen()) { On 2016/10/10 17:57:33, Mustafa Emre Acer wrote: > | is the bitwise operator. You want || instead. Done. https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right): https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:260: BlockInlineInstallFromFullscreenForBrowser) { On 2016/10/10 17:57:33, Mustafa Emre Acer wrote: > nit: Can you add a comment for this test explaining how browser fullscreen is > different from tab fullscreen? > > E.g. > // Prevent inline install while in browser fullscreen mode. Browser fullscreen > // is entered by the user by pressing F11. > > (and same for the next test case) Done. https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:261: GURL install_url = On 2016/10/10 17:57:33, Mustafa Emre Acer wrote: > nit: const GURL install_url = Done. https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:264: AutoAcceptInstall(); On 2016/10/10 18:35:59, Devlin wrote: > On 2016/10/10 17:57:33, Mustafa Emre Acer wrote: > > You probably don't need this since the dialog won't open in the first place? > > I think I suggested this when Ben was talking to me about it. I wanted to make > sure that everything *would* succeed if the error wasn't thrown. Without this > check, there could be a silent test failure where the extension doesn't get > installed, but the reason it doesn't is because we stall at the dialog showing - > when in fact the dialog should never be shown. > > It's probably a little superfluous with the error checks in the js, but it makes > me feel a bit better. Keeping per Devlin's comments. https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:281: GURL install_url = On 2016/10/10 17:57:33, Mustafa Emre Acer wrote: > nit: const GURL install_url = Done. https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:284: AutoAcceptInstall(); On 2016/10/10 17:57:33, Mustafa Emre Acer wrote: > Also not needed? Same comment as above.
Lgtm, but I'm not an owner of this code so I defer the rest to Devlin :) https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right): https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:264: AutoAcceptInstall(); On 2016/10/10 18:35:59, Devlin wrote: > On 2016/10/10 17:57:33, Mustafa Emre Acer wrote: > > You probably don't need this since the dialog won't open in the first place? > > I think I suggested this when Ben was talking to me about it. I wanted to make > sure that everything *would* succeed if the error wasn't thrown. Without this > check, there could be a silent test failure where the extension doesn't get > installed, but the reason it doesn't is because we stall at the dialog showing - > when in fact the dialog should never be shown. > > It's probably a little superfluous with the error checks in the js, but it makes > me feel a bit better. Okay, sounds good. I guess without it the test would timeout instead of failing. https://codereview.chromium.org/2391353003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right): https://codereview.chromium.org/2391353003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:282: // initiated using the browser API. nit: ... initiated by the page using requestFullscreen?
lgtm; nice job!
The CQ bit was checked by ackermanb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Prevent inline install on fullscreen windows. BUG=488143 ========== to ========== Prevent inline install on fullscreen windows. BUG=488143 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Prevent inline install on fullscreen windows. BUG=488143 ========== to ========== Prevent inline install on fullscreen windows. BUG=488143 Committed: https://crrev.com/6ca20846938a643f34ea9ead734c81c4c6ebd74b Cr-Commit-Position: refs/heads/master@{#424254} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6ca20846938a643f34ea9ead734c81c4c6ebd74b Cr-Commit-Position: refs/heads/master@{#424254}
Message was sent while issue was closed.
On 2016/10/10 18:35:59, Devlin (OOO feb 8 and feb 9) wrote: > https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... > File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right): > > https://codereview.chromium.org/2391353003/diff/20001/chrome/browser/extensio... > chrome/browser/extensions/webstore_inline_installer_browsertest.cc:264: > AutoAcceptInstall(); > On 2016/10/10 17:57:33, Mustafa Emre Acer wrote: > > You probably don't need this since the dialog won't open in the first place? > > I think I suggested this when Ben was talking to me about it. I wanted to make > sure that everything *would* succeed if the error wasn't thrown. Without this > check, there could be a silent test failure where the extension doesn't get > installed, but the reason it doesn't is because we stall at the dialog showing - > when in fact the dialog should never be shown. > > It's probably a little superfluous with the error checks in the js, but it makes > me feel a bit better. Now all MacBook users can't inline install any extension if chrome is not on desktop. Fullscreen concepts on Windows and MacBook are different. On the MacBook fullscreen is when the window is maximized and isn't on the desktop.
Message was sent while issue was closed.
Now all MacBook users can't inline install any extension if chrome programm is not on desktop. Fullscreen concepts on Windows and MacBook are different. On the MacBook fullscreen is when the window is maximized and isn't on the desktop. вторник, 11 октября 2016 г., 0:49:19 UTC+3 пользователь commit-bot@chromium.org via codereview.chromium.org написал: > > Patchset 3 (id:??) landed as > https://crrev.com/6ca20846938a643f34ea9ead734c81c4c6ebd74b > Cr-Commit-Position: refs/heads/master@{#424254} > > https://codereview.chromium.org/2391353003/ >
Message was sent while issue was closed.
Now all MacBook users can't inline install any extension if chrome programm is not on desktop. Fullscreen concepts on Windows and MacBook are different. On the MacBook fullscreen is when the window is maximized and isn't on the desktop. вторник, 11 октября 2016 г., 0:49:19 UTC+3 пользователь commit-bot@chromium.org via codereview.chromium.org написал: > > Patchset 3 (id:??) landed as > https://crrev.com/6ca20846938a643f34ea9ead734c81c4c6ebd74b > Cr-Commit-Position: refs/heads/master@{#424254} > > https://codereview.chromium.org/2391353003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2017/02/13 08:57:32, stas3on wrote: > > Now all MacBook users can't inline install any extension if chrome programm is > not on > desktop. > Fullscreen concepts on Windows and MacBook are different. > > On the MacBook fullscreen is when the window is maximized and isn't on the > desktop. Hi, we understand and are working on a solution. Please see https://crbug.com/690523 > > > вторник, 11 октября 2016 г., 0:49:19 UTC+3 пользователь > mailto:commit-bot@chromium.org via http://codereview.chromium.org написал: > > > > Patchset 3 (id:??) landed as > > https://crrev.com/6ca20846938a643f34ea9ead734c81c4c6ebd74b > > Cr-Commit-Position: refs/heads/master@{#424254} > > > > https://codereview.chromium.org/2391353003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
