Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2512553007:
Add GetIsPinnedToTaskbarState() (Closed)
|
Created:
4 years, 1 month ago by Patrick Monette Modified:
4 years ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd GetIsPinnedToTaskbarState()
Adds the more general function to get the pinned state and refactor
the 2 usages of ShellHandler.
BUG=667335
Committed: https://crrev.com/a9831524bf640f9a3eb7842e83fe6c4d3d13652e
Cr-Commit-Position: refs/heads/master@{#434254}
Patch Set 1 #
Total comments: 10
Patch Set 2 : grt's comments #
Total comments: 4
Patch Set 3 : More nits #
Total comments: 2
Patch Set 4 : Added comment #Patch Set 5 : Merge #Patch Set 6 : Fixed #
Messages
Total messages: 27 (12 generated)
pmonette@chromium.org changed reviewers: + grt@chromium.org
PTAL. https://codereview.chromium.org/2512553007/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2512553007/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:116: UMA_ANDROID_ARM_FPU_VFPV3_D16, // The ARM CPU only supports vfpv3-d16. Fixing a lint error. https://codereview.chromium.org/2512553007/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/2512553007/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:14: #include <memory> Fixing lint errors.
looks good. is there a bug number this could be associated with in the CL description? https://codereview.chromium.org/2512553007/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/2512553007/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:464: new IsPinnedToTaskbarHelper(error_callback, result_callback); // Self-deleting when the ShellHandler completes. https://codereview.chromium.org/2512553007/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.h (right): https://codereview.chromium.org/2512553007/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.h:67: void GetIsPinnedToTaskbarState(ConnectionErrorCallback on_error_callback, does this need to be called on the IO thread? if so, please document as such. if not, should the metrics code and/or comments be updated? https://codereview.chromium.org/2512553007/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.h:67: void GetIsPinnedToTaskbarState(ConnectionErrorCallback on_error_callback, nit: pass callbacks via constref everywhere https://codereview.chromium.org/2512553007/diff/1/chrome/browser/ui/webui/wel... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2512553007/diff/1/chrome/browser/ui/webui/wel... chrome/browser/ui/webui/welcome_win10_handler.cc:90: OnIsPinnedToTaskbarDetermined(succeeded && is_pinned_to_taskbar); should this be !succeeded || is_pinned_to_taskbar for consistency with the way connection errors are handled above? if not, please explain in a comment either here or up on line 77 (or both!).
Description was changed from ========== Add GetIsPinnedToTaskbarState() Adds the more general function to get the pinned state and refactor the 2 usages of ShellHandler. ========== to ========== Add GetIsPinnedToTaskbarState() Adds the more general function to get the pinned state and refactor the 2 usages of ShellHandler. BUG=667335 ==========
Thanks Greg! PTAnL https://codereview.chromium.org/2512553007/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/2512553007/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:464: new IsPinnedToTaskbarHelper(error_callback, result_callback); On 2016/11/21 10:45:31, grt (UTC plus 1) wrote: > // Self-deleting when the ShellHandler completes. Done. https://codereview.chromium.org/2512553007/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.h (right): https://codereview.chromium.org/2512553007/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.h:67: void GetIsPinnedToTaskbarState(ConnectionErrorCallback on_error_callback, On 2016/11/21 10:45:31, grt (UTC plus 1) wrote: > nit: pass callbacks via constref everywhere Done. https://codereview.chromium.org/2512553007/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.h:67: void GetIsPinnedToTaskbarState(ConnectionErrorCallback on_error_callback, On 2016/11/21 10:45:31, grt (UTC plus 1) wrote: > does this need to be called on the IO thread? if so, please document as such. if > not, should the metrics code and/or comments be updated? I don't think it does since it's not blocking IO. I've removed the DCHECK in chrome_browser_main_extra_parts_metrics.cc https://codereview.chromium.org/2512553007/diff/1/chrome/browser/ui/webui/wel... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2512553007/diff/1/chrome/browser/ui/webui/wel... chrome/browser/ui/webui/welcome_win10_handler.cc:90: OnIsPinnedToTaskbarDetermined(succeeded && is_pinned_to_taskbar); On 2016/11/21 10:45:31, grt (UTC plus 1) wrote: > should this be !succeeded || is_pinned_to_taskbar for consistency with the way > connection errors are handled above? if not, please explain in a comment either > here or up on line 77 (or both!). Good catch. Done.
lgtm w/ two nits https://codereview.chromium.org/2512553007/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2512553007/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:330: enum Result { NOT_PINNED, PINNED, FAILURE, NUM_RESULTS }; nit: since this must be in sync with histograms.xml and the items must not have their values modified, i think it's nice to give them explicit values here and add a comment saying not to reorder: // Used for histograms; do not reorder. enum Result { NOT_PINNED = 0, PINNED = 1, FAILURE = 2, NUM_RESULTS }; https://codereview.chromium.org/2512553007/diff/20001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/2512553007/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:438: // complexity of managing the lifetime of |shell_handler_|. nit: "...of a UtilityProcessMojoClient."
pmonette@chromium.org changed reviewers: + asvitkine@chromium.org, michaelpg@chromium.org
Thanks! asvitkine@ PTAL at chrome_browser_main_extra_parts_metrics.cc michaelpg@ PTAL at chrome/browser/ui/webui/* This is only a refactoring patch. https://codereview.chromium.org/2512553007/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2512553007/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:330: enum Result { NOT_PINNED, PINNED, FAILURE, NUM_RESULTS }; On 2016/11/22 10:06:41, grt (UTC plus 1) wrote: > nit: since this must be in sync with histograms.xml and the items must not have > their values modified, i think it's nice to give them explicit values here and > add a comment saying not to reorder: > // Used for histograms; do not reorder. > enum Result { > NOT_PINNED = 0, > PINNED = 1, > FAILURE = 2, > NUM_RESULTS > }; Done. https://codereview.chromium.org/2512553007/diff/20001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/2512553007/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:438: // complexity of managing the lifetime of |shell_handler_|. On 2016/11/22 10:06:41, grt (UTC plus 1) wrote: > nit: "...of a UtilityProcessMojoClient." Done.
c/b/ui/webui lgtm
Seems you're missing the histograms.xml change?
On 2016/11/23 17:56:39, Alexei Svitkine (slow) wrote: > Seems you're missing the histograms.xml change? Nevermind, I see you're just moving the code around.
lgtm % comment https://codereview.chromium.org/2512553007/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/2512553007/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:482: base::Unretained(this))); Why base::Unretained()? Is there a guarantee that |this| won't be destroyed before the callback runs? If so, mention it in a comment. Otherwise, use a WeakPtrFactory.
Patchset #4 (id:60001) has been deleted
Thanks! https://codereview.chromium.org/2512553007/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/2512553007/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:482: base::Unretained(this))); On 2016/11/23 17:59:53, Alexei Svitkine (slow) wrote: > Why base::Unretained()? > > Is there a guarantee that |this| won't be destroyed before the callback runs? If > so, mention it in a comment. > > Otherwise, use a WeakPtrFactory. Added a comment
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, asvitkine@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2512553007/#ps80001 (title: "Added comment")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, michaelpg@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2512553007/#ps120001 (title: "Fixed")
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": 120001, "attempt_start_ts": 1479930951073080,
"parent_rev": "be7c7dc2caa165a16e9ee309f4e8244bc8df13de", "commit_rev":
"0a07190364628355d10e0b7cc10a4831a1fcdee8"}
Message was sent while issue was closed.
Description was changed from ========== Add GetIsPinnedToTaskbarState() Adds the more general function to get the pinned state and refactor the 2 usages of ShellHandler. BUG=667335 ========== to ========== Add GetIsPinnedToTaskbarState() Adds the more general function to get the pinned state and refactor the 2 usages of ShellHandler. BUG=667335 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add GetIsPinnedToTaskbarState() Adds the more general function to get the pinned state and refactor the 2 usages of ShellHandler. BUG=667335 ========== to ========== Add GetIsPinnedToTaskbarState() Adds the more general function to get the pinned state and refactor the 2 usages of ShellHandler. BUG=667335 Committed: https://crrev.com/a9831524bf640f9a3eb7842e83fe6c4d3d13652e Cr-Commit-Position: refs/heads/master@{#434254} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a9831524bf640f9a3eb7842e83fe6c4d3d13652e Cr-Commit-Position: refs/heads/master@{#434254} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
