|
|
Created:
4 years, 1 month ago by Avi (use Gerrit) Modified:
4 years, 1 month ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLog to the console when a dialog is suppressed or would be.
BUG=629964
Committed: https://crrev.com/7484f1a2039a73e59ae812d6a15e21023c9827c7
Cr-Commit-Position: refs/heads/master@{#431457}
Patch Set 1 #Patch Set 2 : phrasing! #
Total comments: 8
Patch Set 3 : rewording #Patch Set 4 : drop the second one #Messages
Total messages: 22 (14 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...
avi@chromium.org changed reviewers: + dknox@chromium.org, ojan@chromium.org, rbyers@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2491163002/diff/20001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc (right): https://codereview.chromium.org/2491163002/diff/20001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:94: "Please do not use dialogs to force browser windows to the front."); Nit: add chromestatus.com feature link for now (we can link to devrel docs, samples, etc. from there). https://codereview.chromium.org/2491163002/diff/20001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:94: "Please do not use dialogs to force browser windows to the front."); Is there some way for developers to actually do what you suggest in the last sentence here? I.e. does the page visibility API give them the exact information they need to know when not to prompt? Or is the only real solution to not use the prompt API at all? If there's not great guidance here, then you might just want to omit that last sentence (maybe it's legitimate to still use prompt but just handle the empty return?). https://codereview.chromium.org/2491163002/diff/20001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:103: "the front."); Do you have UMA data on how often this occurs? We're pretty sensitive to console warning spam - we know it causes warning blindness. Also are you confident enough in the consensus here that we can say "will soon be"... or should it be "may"? Again, should ideally have a link for more details - chromestatus feature for now should be fine.
In the email thread "Making official Chromium policy" that you're on, I've been trying to get some kind of article, and a link to that article would be perfect for this. Until that happens, I'll link chromestatus. I'll make an entry, link it here, and send out the intent.
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...
https://codereview.chromium.org/2491163002/diff/20001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc (right): https://codereview.chromium.org/2491163002/diff/20001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:94: "Please do not use dialogs to force browser windows to the front."); On 2016/11/10 04:18:39, Rick Byers wrote: > Nit: add http://chromestatus.com feature link for now (we can link to devrel docs, > samples, etc. from there). Done. https://codereview.chromium.org/2491163002/diff/20001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:94: "Please do not use dialogs to force browser windows to the front."); On 2016/11/10 04:18:39, Rick Byers wrote: > Is there some way for developers to actually do what you suggest in the last > sentence here? I.e. does the page visibility API give them the exact > information they need to know when not to prompt? Or is the only real solution > to not use the prompt API at all? If there's not great guidance here, then you > might just want to omit that last sentence (maybe it's legitimate to still use > prompt but just handle the empty return?). What they should do is not use prompt() untriggered by user interaction; if the user is interacting with the page, it'll be foremost. Let me try rephrasing. https://codereview.chromium.org/2491163002/diff/20001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:103: "the front."); On 2016/11/10 04:18:39, Rick Byers wrote: > Do you have UMA data on how often this occurs? No, I don't, unfortunately. Do you want to not warn in this situation?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM if you split out the 2nd warning https://codereview.chromium.org/2491163002/diff/20001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc (right): https://codereview.chromium.org/2491163002/diff/20001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:94: "Please do not use dialogs to force browser windows to the front."); On 2016/11/10 21:04:30, Avi wrote: > On 2016/11/10 04:18:39, Rick Byers wrote: > > Nit: add http://chromestatus.com feature link for now (we can link to devrel > docs, > > samples, etc. from there). > > Done. Sounds good - thanks! https://codereview.chromium.org/2491163002/diff/20001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:103: "the front."); On 2016/11/10 21:04:30, Avi wrote: > On 2016/11/10 04:18:39, Rick Byers wrote: > > Do you have UMA data on how often this occurs? > > No, I don't, unfortunately. > > Do you want to not warn in this situation? I guess I'd rather discuss this one separately - ideally on an "intent to deprecate" thread. Then we could get agreement on a specific removal plan and include a date/milestone etc. Otherwise we know the impact to the warning won't be as high.
The CQ bit was checked by avi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2491163002/#ps60001 (title: "drop the second one")
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Log to the console when a dialog is suppressed or would be. BUG=629964 ========== to ========== Log to the console when a dialog is suppressed or would be. BUG=629964 Committed: https://crrev.com/7484f1a2039a73e59ae812d6a15e21023c9827c7 Cr-Commit-Position: refs/heads/master@{#431457} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7484f1a2039a73e59ae812d6a15e21023c9827c7 Cr-Commit-Position: refs/heads/master@{#431457} |