Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11280112:
UMA for Windows 8 Secondary Tile pinning/unpinning user actions (Closed)
|
Created:
8 years, 1 month ago by tapted Modified:
8 years ago CC:
chromium-reviews, koz (OOO until 15th September), jeremya Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionUMA for Windows 8 Secondary Tile pinning/unpinning user actions
triggered from hotdog -> Bookmarks when in Metro mode.
To perform the metrics recording, this CL includes changes
to provide a callback mechanism for the asynchronous tile
creation in metro_driver.dll to report back the result of
the use gesture to the browser process. Note that both
metro_driver.dll and chrome.dll link statically to base, so
they have distinct data segments holding UMA histograms,
hence the callback.
BUG=160840
TEST=In Windows 8 Metro Mode, Hotdog -> Bookmarks -> Pin to start page:
- pin and cancel (escape/click/touch off popup)
- pin and confirm
- (when pinned) unpin and cancel
- unpin and confirm
Above should all work (and report UMA histogram data).
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170916
Patch Set 1 #Patch Set 2 : use histograms to track pin lifecycle #Patch Set 3 : need to decouple create and delete #Patch Set 4 : base is statically linked to metro_driver.dll -- use a callback #Patch Set 5 : no longer need to share this histogram name constant #
Total comments: 19
Patch Set 6 : reviewer comments #
Total comments: 6
Patch Set 7 : **fails presubmit** make secondary_tile_types.h, +review comments #
Total comments: 6
Patch Set 8 : types back to base/win/metro.h, pass callback by const-reference, Uma, reviewer comments #
Total comments: 5
Patch Set 9 : reviewer comments #
Total comments: 2
Patch Set 10 : update member function comment #
Total comments: 2
Patch Set 11 : add TODO #
Messages
Total messages: 23 (0 generated)
@benwells for initial review (and to possibly suggest some windows 8 gurus)
Great to see this all working and almost there! https://codereview.chromium.org/11280112/diff/4005/base/win/metro.h File base/win/metro.h (right): https://codereview.chromium.org/11280112/diff/4005/base/win/metro.h#newcode40 base/win/metro.h:40: // Enum values for UMA histogram reporting of site-specific tile pinning. Does this stuff all have to go in this file? It doesn't feel right going in base. https://codereview.chromium.org/11280112/diff/4005/chrome/browser/ui/metro_pi... File chrome/browser/ui/metro_pin_tab_helper_win.cc (right): https://codereview.chromium.org/11280112/diff/4005/chrome/browser/ui/metro_pi... chrome/browser/ui/metro_pin_tab_helper_win.cc:166: void PinPageReportUMACallback(void* callback_data, callback_data doesn't seem to serve any purpose. Why plumb it all the way through? https://codereview.chromium.org/11280112/diff/4005/win8/metro_driver/secondar... File win8/metro_driver/secondary_tile.cc (right): https://codereview.chromium.org/11280112/diff/4005/win8/metro_driver/secondar... win8/metro_driver/secondary_tile.cc:30: : type_(type), callback_(callback), callback_data_(callback_data) { Nit: indenting here is off. THe : should line up with the parameters to the constructor. https://codereview.chromium.org/11280112/diff/4005/win8/metro_driver/secondar... win8/metro_driver/secondary_tile.cc:32: mswr::ComPtr<RequestDoneType> handler(mswr::Callback<RequestDoneType>( Can you move this logic into another function? It would be cleaner, remove any question of weird problems, and would make the calling code more obvious. It's also agains the google style guide to do 'work' in the constructor: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Doing_... https://codereview.chromium.org/11280112/diff/4005/win8/metro_driver/secondar... win8/metro_driver/secondary_tile.cc:48: HRESULT TileRequestDone::respond(winfoundtn::IAsyncOperation<bool>* async, Is there a reason to have the constructor inline and this function out of line? Consistency would be good. https://codereview.chromium.org/11280112/diff/4005/win8/metro_driver/secondar... win8/metro_driver/secondary_tile.cc:53: delete this; Hmm. I assume there is some reason to copy the members, delete here and then carry on, as it is a bit weird. Can you put a comment explaining why you're doing this? https://codereview.chromium.org/11280112/diff/4005/win8/metro_driver/secondar... win8/metro_driver/secondary_tile.cc:213: base::win::MetroUnPinFromStartScreen check = &MetroUnPinFromStartScreen; Um ... I haven't seen anything like this in chrome code. I can see what you are doing, but maybe it would be better done in a unit test. This feels a little too tricky.
Let me know what you think about the plans below for moving things out of base. https://codereview.chromium.org/11280112/diff/4005/base/win/metro.h File base/win/metro.h (right): https://codereview.chromium.org/11280112/diff/4005/base/win/metro.h#newcode40 base/win/metro.h:40: // Enum values for UMA histogram reporting of site-specific tile pinning. On 2012/11/27 08:56:39, benwells wrote: > Does this stuff all have to go in this file? It doesn't feel right going in > base. It needs to go somewhere shared between /win8 and /chrome/browser/ui.. Currently, viable options (i.e. current viable dependencies of both) are: * /base, and * /chrome/common/common_constants, /base/win/metro.h currently seems to be the most common way for things to be shared between the metro driver and the win8 parts of the browser. There is also: * /ui/metro_viewer/[metro_viewer_messages.h] An option [option "1"] in the same vein as this would be to make: * /ui/metro_uma/ * /ui/metro_uma/metro_uma.gyp * /ui/metro_uma/metro_uma_types.h That feels a bit heavyweight. The "right" solution to me feels like: * /win8/common/ * /win8/common/win8_common.gyp (or win8/win8_common.gyp .. or .gypi ) Then the stuff currently in /base/win/metro.h moves to /win8/common/metro.h. That feels like a separate CL though [option "2"], and there might be a good reason for having some metro stuff in /base. But then, for this, we would just add /win8/common/secondary_tile_types.h . Alternatively, [option "3"] I can get that ball rolling in this CL. That is, start with /win8/common/{win8_common.gypi,secondary_tile_types.h}. And add OS=="win" ==> win8_common.gypi dependency to chrome_browser.gypi, right under '../ui/metro_viewer/metro_viewer.gyp:metro_viewer', then things can start moving out of /base/win in follow-ups. Or try out a CL for moving everything first [option "2"], and land it before this (assuming it's feasible). An [option "4"]: * These enums are not necessarily UMA-specific: this comment could rightly drop the UMA wording and maybe feel less out of place. An [option "1.1"]: * Do "1", but put it in /win8/metro_uma And there's always ["option 0"]: no-op ;-). Nothing done yet -- WDYT? https://codereview.chromium.org/11280112/diff/4005/chrome/browser/ui/metro_pi... File chrome/browser/ui/metro_pin_tab_helper_win.cc (right): https://codereview.chromium.org/11280112/diff/4005/chrome/browser/ui/metro_pi... chrome/browser/ui/metro_pin_tab_helper_win.cc:166: void PinPageReportUMACallback(void* callback_data, On 2012/11/27 08:56:39, benwells wrote: > callback_data doesn't seem to serve any purpose. Why plumb it all the way > through? I think my fingers just balked at making a callback API with no way to pass through user data ;-). Since, that limits a callback to accessing static/global data only. Which, for UMA, it is. So, I didn't end up using it, even though the caller -- MetroPinTabHelper -- is per-tab. I'll cut it out. [Done.] https://codereview.chromium.org/11280112/diff/4005/win8/metro_driver/secondar... File win8/metro_driver/secondary_tile.cc (right): https://codereview.chromium.org/11280112/diff/4005/win8/metro_driver/secondar... win8/metro_driver/secondary_tile.cc:30: : type_(type), callback_(callback), callback_data_(callback_data) { On 2012/11/27 08:56:39, benwells wrote: > Nit: indenting here is off. THe : should line up with the parameters to the > constructor. Done. https://codereview.chromium.org/11280112/diff/4005/win8/metro_driver/secondar... win8/metro_driver/secondary_tile.cc:32: mswr::ComPtr<RequestDoneType> handler(mswr::Callback<RequestDoneType>( On 2012/11/27 08:56:39, benwells wrote: > Can you move this logic into another function? It would be cleaner, remove any > question of weird problems, and would make the calling code more obvious. > > It's also agains the google style guide to do 'work' in the constructor: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Doing_... Done. https://codereview.chromium.org/11280112/diff/4005/win8/metro_driver/secondar... win8/metro_driver/secondary_tile.cc:48: HRESULT TileRequestDone::respond(winfoundtn::IAsyncOperation<bool>* async, On 2012/11/27 08:56:39, benwells wrote: > Is there a reason to have the constructor inline and this function out of line? > Consistency would be good. No particular reason. (It met the style guide :) -- 10 lines, no loops/switch statements). I've moved it out. [Done.] https://codereview.chromium.org/11280112/diff/4005/win8/metro_driver/secondar... win8/metro_driver/secondary_tile.cc:53: delete this; On 2012/11/27 08:56:39, benwells wrote: > Hmm. I assume there is some reason to copy the members, delete here and then > carry on, as it is a bit weird. Can you put a comment explaining why you're > doing this? Just defensive programming - more from a mindset like ~"what if that callback throws an exception". But while we're not making a generic callback API it's not really an issue, so I've changed it to be less weird. [Done.] https://codereview.chromium.org/11280112/diff/4005/win8/metro_driver/secondar... win8/metro_driver/secondary_tile.cc:213: base::win::MetroUnPinFromStartScreen check = &MetroUnPinFromStartScreen; On 2012/11/27 08:56:39, benwells wrote: > Um ... I haven't seen anything like this in chrome code. I can see what you are > doing, but maybe it would be better done in a unit test. This feels a little too > tricky. In a unit test it would only check the prototype declaration. The actual definition in the DLL could still be an override, resulting in GetProcAddress failing at runtime. (Although if the unit test _called_ the function, the linker should pick it up). I actually hit this -- accidentally changed the signature IsPinned..(), and the hotdog menu would no longer load. So it can be a subtle problem if someone changes the typedef and misses one of the definitions. But, you're right about nobody using the `(void)local_arg` technique in chrome (I did a grep..). Maybe I just delete it [Done.].. but let me know if I should think more.
I've added mad@ as a reviewer as I heard a rumour he has done metro UMA reporting. Also +cc cpu@, the chrome win 8 expert. https://codereview.chromium.org/11280112/diff/4005/base/win/metro.h File base/win/metro.h (right): https://codereview.chromium.org/11280112/diff/4005/base/win/metro.h#newcode40 base/win/metro.h:40: // Enum values for UMA histogram reporting of site-specific tile pinning. On 2012/11/28 00:33:43, tapted wrote: > On 2012/11/27 08:56:39, benwells wrote: > > Does this stuff all have to go in this file? It doesn't feel right going in > > base. > > It needs to go somewhere shared between /win8 and /chrome/browser/ui.. > > Currently, viable options (i.e. current viable dependencies of both) are: > * /base, and > * /chrome/common/common_constants, > > /base/win/metro.h currently seems to be the most common way for things to be > shared between the metro driver and the win8 parts of the browser. > > There is also: > * /ui/metro_viewer/[metro_viewer_messages.h] > > An option [option "1"] in the same vein as this would be to make: > * /ui/metro_uma/ > * /ui/metro_uma/metro_uma.gyp > * /ui/metro_uma/metro_uma_types.h > > That feels a bit heavyweight. > > The "right" solution to me feels like: > * /win8/common/ > * /win8/common/win8_common.gyp (or win8/win8_common.gyp .. or .gypi ) > > Then the stuff currently in /base/win/metro.h moves to /win8/common/metro.h. > That feels like a separate CL though [option "2"], and there might be a good > reason for having some metro stuff in /base. But then, for this, we would just > add /win8/common/secondary_tile_types.h . > > Alternatively, [option "3"] I can get that ball rolling in this CL. That is, > start with /win8/common/{win8_common.gypi,secondary_tile_types.h}. And add > OS=="win" ==> win8_common.gypi dependency to chrome_browser.gypi, right under > '../ui/metro_viewer/metro_viewer.gyp:metro_viewer', > then things can start moving out of /base/win in follow-ups. > > Or try out a CL for moving everything first [option "2"], and land it before > this (assuming it's feasible). > > An [option "4"]: > * These enums are not necessarily UMA-specific: this comment could rightly drop > the UMA wording and maybe feel less out of place. > > An [option "1.1"]: > * Do "1", but put it in /win8/metro_uma > > And there's always ["option 0"]: no-op ;-). > > Nothing done yet -- WDYT? I was referring to all the secondary tile stuff you're adding, not just the UMA stuff. Today I chatted with cpu@ about this and he also wasn't keen to put secondary related tile stuff into this file. Why not just put it in win8/metro_driver/secondary_tile.h and refer to that from metro_pin_tab_helper? https://codereview.chromium.org/11280112/diff/4005/chrome/browser/ui/metro_pi... File chrome/browser/ui/metro_pin_tab_helper_win.cc (right): https://codereview.chromium.org/11280112/diff/4005/chrome/browser/ui/metro_pi... chrome/browser/ui/metro_pin_tab_helper_win.cc:166: void PinPageReportUMACallback(void* callback_data, On 2012/11/28 00:33:43, tapted wrote: > On 2012/11/27 08:56:39, benwells wrote: > > callback_data doesn't seem to serve any purpose. Why plumb it all the way > > through? > > I think my fingers just balked at making a callback API with no way to pass > through user data ;-). Since, that limits a callback to accessing static/global > data only. Which, for UMA, it is. So, I didn't end up using it, even though the > caller -- MetroPinTabHelper -- is per-tab. > > I'll cut it out. [Done.] If you really want to pass callback data through you can always curry it in with base::bind later on. That's the way we roll. https://codereview.chromium.org/11280112/diff/15002/base/win/metro.h File base/win/metro.h (right): https://codereview.chromium.org/11280112/diff/15002/base/win/metro.h#newcode121 base/win/metro.h:121: typedef void (*MetroPinResultCallback)(MetroSecondaryTilePinState result); We should use something like base::Callback<void(MetroSecondaryTilePinState)> for this. https://codereview.chromium.org/11280112/diff/15002/win8/metro_driver/seconda... File win8/metro_driver/secondary_tile.cc (right): https://codereview.chromium.org/11280112/diff/15002/win8/metro_driver/seconda... win8/metro_driver/secondary_tile.cc:28: } Blank line after. Can have {} on one line. https://codereview.chromium.org/11280112/diff/15002/win8/metro_driver/seconda... win8/metro_driver/secondary_tile.cc:181: (new TileRequestDone(TileRequestDone::PIN, Please add a comment here about this object's lifetime.
https://codereview.chromium.org/11280112/diff/4005/base/win/metro.h File base/win/metro.h (right): https://codereview.chromium.org/11280112/diff/4005/base/win/metro.h#newcode40 base/win/metro.h:40: // Enum values for UMA histogram reporting of site-specific tile pinning. On 2012/11/29 04:19:46, benwells wrote: > On 2012/11/28 00:33:43, tapted wrote: > > On 2012/11/27 08:56:39, benwells wrote: > > > Does this stuff all have to go in this file? It doesn't feel right going in > > > base. > > > > It needs to go somewhere shared between /win8 and /chrome/browser/ui.. > > > > Currently, viable options (i.e. current viable dependencies of both) are: > > * /base, and > > * /chrome/common/common_constants, > > > > /base/win/metro.h currently seems to be the most common way for things to be > > shared between the metro driver and the win8 parts of the browser. > > > > There is also: > > * /ui/metro_viewer/[metro_viewer_messages.h] > > > > An option [option "1"] in the same vein as this would be to make: > > * /ui/metro_uma/ > > * /ui/metro_uma/metro_uma.gyp > > * /ui/metro_uma/metro_uma_types.h > > > > That feels a bit heavyweight. > > > > The "right" solution to me feels like: > > * /win8/common/ > > * /win8/common/win8_common.gyp (or win8/win8_common.gyp .. or .gypi ) > > > > Then the stuff currently in /base/win/metro.h moves to /win8/common/metro.h. > > That feels like a separate CL though [option "2"], and there might be a good > > reason for having some metro stuff in /base. But then, for this, we would just > > add /win8/common/secondary_tile_types.h . > > > > Alternatively, [option "3"] I can get that ball rolling in this CL. That is, > > start with /win8/common/{win8_common.gypi,secondary_tile_types.h}. And add > > OS=="win" ==> win8_common.gypi dependency to chrome_browser.gypi, right under > > '../ui/metro_viewer/metro_viewer.gyp:metro_viewer', > > then things can start moving out of /base/win in follow-ups. > > > > Or try out a CL for moving everything first [option "2"], and land it before > > this (assuming it's feasible). > > > > An [option "4"]: > > * These enums are not necessarily UMA-specific: this comment could rightly > drop > > the UMA wording and maybe feel less out of place. > > > > An [option "1.1"]: > > * Do "1", but put it in /win8/metro_uma > > > > And there's always ["option 0"]: no-op ;-). > > > > Nothing done yet -- WDYT? > > I was referring to all the secondary tile stuff you're adding, not just the UMA > stuff. Today I chatted with cpu@ about this and he also wasn't keen to put > secondary related tile stuff into this file. > > Why not just put it in win8/metro_driver/secondary_tile.h and refer to that from > metro_pin_tab_helper? ** Presubmit ERRORS ** You added one or more #includes that violate checkdeps rules. chrome\browser\ui\metro_pin_tab_helper_win.cc Illegal include: "win8/metro_driver/secondary_tile_types.h" Because of no rule applying. "One does not simply #include in Chrome..." ;) I've uploaded a patch with presubmit hooks skippped to make sure this is what you had in mind. I'll look into policies/procedures around this tomorrow. Perhaps, for approval, there is a "right" way to do it (maybe along the lines of option [3]). https://codereview.chromium.org/11280112/diff/15002/base/win/metro.h File base/win/metro.h (right): https://codereview.chromium.org/11280112/diff/15002/base/win/metro.h#newcode121 base/win/metro.h:121: typedef void (*MetroPinResultCallback)(MetroSecondaryTilePinState result); On 2012/11/29 04:19:46, benwells wrote: > We should use something like base::Callback<void(MetroSecondaryTilePinState)> > for this. Done. https://codereview.chromium.org/11280112/diff/15002/win8/metro_driver/seconda... File win8/metro_driver/secondary_tile.cc (right): https://codereview.chromium.org/11280112/diff/15002/win8/metro_driver/seconda... win8/metro_driver/secondary_tile.cc:28: } On 2012/11/29 04:19:46, benwells wrote: > Blank line after. Can have {} on one line. Done. https://codereview.chromium.org/11280112/diff/15002/win8/metro_driver/seconda... win8/metro_driver/secondary_tile.cc:181: (new TileRequestDone(TileRequestDone::PIN, On 2012/11/29 04:19:46, benwells wrote: > Please add a comment here about this object's lifetime. Done.
we should not add a dependency from src\chrome to src\win8. If you want to keep the pin states in win8, if they are used for other things than UMA, then we can have them in win8 but have corresponding UMA identifiers for them in base/win/metro.h and have a converter in win8 code... As mentioned in the comments below, if these are only used for UMA, I would keep them in base/win/metro.h, but name them appropriately... If/Once we start supporting UMA straight from src\win8 metro_driver.dll, then we can move these IDs to src\win8. BYE MAD https://codereview.chromium.org/11280112/diff/4005/base/win/metro.h File base/win/metro.h (right): https://codereview.chromium.org/11280112/diff/4005/base/win/metro.h#newcode40 base/win/metro.h:40: // Enum values for UMA histogram reporting of site-specific tile pinning. On 2012/11/29 07:19:27, tapted wrote: > On 2012/11/29 04:19:46, benwells wrote: > > On 2012/11/28 00:33:43, tapted wrote: > > > On 2012/11/27 08:56:39, benwells wrote: > > > > Does this stuff all have to go in this file? It doesn't feel right going > in > > > > base. > > > > > > It needs to go somewhere shared between /win8 and /chrome/browser/ui.. > > > > > > Currently, viable options (i.e. current viable dependencies of both) are: > > > * /base, and > > > * /chrome/common/common_constants, > > > > > > /base/win/metro.h currently seems to be the most common way for things to be > > > shared between the metro driver and the win8 parts of the browser. > > > > > > There is also: > > > * /ui/metro_viewer/[metro_viewer_messages.h] > > > > > > An option [option "1"] in the same vein as this would be to make: > > > * /ui/metro_uma/ > > > * /ui/metro_uma/metro_uma.gyp > > > * /ui/metro_uma/metro_uma_types.h > > > > > > That feels a bit heavyweight. > > > > > > The "right" solution to me feels like: > > > * /win8/common/ > > > * /win8/common/win8_common.gyp (or win8/win8_common.gyp .. or .gypi ) > > > > > > Then the stuff currently in /base/win/metro.h moves to /win8/common/metro.h. > > > That feels like a separate CL though [option "2"], and there might be a good > > > reason for having some metro stuff in /base. But then, for this, we would > just > > > add /win8/common/secondary_tile_types.h . > > > > > > Alternatively, [option "3"] I can get that ball rolling in this CL. That is, > > > start with /win8/common/{win8_common.gypi,secondary_tile_types.h}. And add > > > OS=="win" ==> win8_common.gypi dependency to chrome_browser.gypi, right > under > > > '../ui/metro_viewer/metro_viewer.gyp:metro_viewer', > > > then things can start moving out of /base/win in follow-ups. > > > > > > Or try out a CL for moving everything first [option "2"], and land it before > > > this (assuming it's feasible). > > > > > > An [option "4"]: > > > * These enums are not necessarily UMA-specific: this comment could rightly > > drop > > > the UMA wording and maybe feel less out of place. > > > > > > An [option "1.1"]: > > > * Do "1", but put it in /win8/metro_uma > > > > > > And there's always ["option 0"]: no-op ;-). > > > > > > Nothing done yet -- WDYT? > > > > I was referring to all the secondary tile stuff you're adding, not just the > UMA > > stuff. Today I chatted with cpu@ about this and he also wasn't keen to put > > secondary related tile stuff into this file. > > > > Why not just put it in win8/metro_driver/secondary_tile.h and refer to that > from > > metro_pin_tab_helper? > > ** Presubmit ERRORS ** > You added one or more #includes that violate checkdeps rules. > chrome\browser\ui\metro_pin_tab_helper_win.cc > Illegal include: "win8/metro_driver/secondary_tile_types.h" > Because of no rule applying. > > "One does not simply #include in Chrome..." ;) > > I've uploaded a patch with presubmit hooks skippped to make sure this is what > you had in mind. I'll look into policies/procedures around this tomorrow. > Perhaps, for approval, there is a "right" way to do it (maybe along the lines of > option [3]). If these are specific to UMA I would leave them here and put UMA in the name somewhere... https://codereview.chromium.org/11280112/diff/5013/chrome/chrome_browser_ui.gypi File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/11280112/diff/5013/chrome/chrome_browser_ui.g... chrome/chrome_browser_ui.gypi:2658: '../win8/metro_driver/secondary_tile_types.h', We should not include stuff from the win8 folder here... https://codereview.chromium.org/11280112/diff/5013/win8/metro_driver/metro_dr... File win8/metro_driver/metro_driver.gyp (right): https://codereview.chromium.org/11280112/diff/5013/win8/metro_driver/metro_dr... win8/metro_driver/metro_driver.gyp:101: 'secondary_tile_types.h', I don't see this file in your CL? https://codereview.chromium.org/11280112/diff/5013/win8/metro_driver/secondar... File win8/metro_driver/secondary_tile.cc (right): https://codereview.chromium.org/11280112/diff/5013/win8/metro_driver/secondar... win8/metro_driver/secondary_tile.cc:200: // Ensure function signature is correct. I don't understand this comment...
On 2012/11/29 15:26:18, MAD wrote: > we should not add a dependency from src\chrome to src\win8. If you want to keep > the pin states in win8, if they are used for other things than UMA, then we can > have them in win8 but have corresponding UMA identifiers for them in > base/win/metro.h and have a converter in win8 code... > > As mentioned in the comments below, if these are only used for UMA, I would keep > them in base/win/metro.h, but name them appropriately... > > If/Once we start supporting UMA straight from src\win8 metro_driver.dll, then we > can move these IDs to src\win8. > > BYE > MAD OK, I stand corrected about having this stuff in base. It feels wrong to me to put things for UMA stats into base, but I have learnt we don't want a build time dependency from chrome to win8 (just the existing run time dependency), and there isn't anywhere else for it ... > > https://codereview.chromium.org/11280112/diff/4005/base/win/metro.h > File base/win/metro.h (right): > > https://codereview.chromium.org/11280112/diff/4005/base/win/metro.h#newcode40 > base/win/metro.h:40: // Enum values for UMA histogram reporting of site-specific > tile pinning. > On 2012/11/29 07:19:27, tapted wrote: > > On 2012/11/29 04:19:46, benwells wrote: > > > On 2012/11/28 00:33:43, tapted wrote: > > > > On 2012/11/27 08:56:39, benwells wrote: > > > > > Does this stuff all have to go in this file? It doesn't feel right going > > in > > > > > base. > > > > > > > > It needs to go somewhere shared between /win8 and /chrome/browser/ui.. > > > > > > > > Currently, viable options (i.e. current viable dependencies of both) are: > > > > * /base, and > > > > * /chrome/common/common_constants, > > > > > > > > /base/win/metro.h currently seems to be the most common way for things to > be > > > > shared between the metro driver and the win8 parts of the browser. > > > > > > > > There is also: > > > > * /ui/metro_viewer/[metro_viewer_messages.h] > > > > > > > > An option [option "1"] in the same vein as this would be to make: > > > > * /ui/metro_uma/ > > > > * /ui/metro_uma/metro_uma.gyp > > > > * /ui/metro_uma/metro_uma_types.h > > > > > > > > That feels a bit heavyweight. > > > > > > > > The "right" solution to me feels like: > > > > * /win8/common/ > > > > * /win8/common/win8_common.gyp (or win8/win8_common.gyp .. or .gypi ) > > > > > > > > Then the stuff currently in /base/win/metro.h moves to > /win8/common/metro.h. > > > > That feels like a separate CL though [option "2"], and there might be a > good > > > > reason for having some metro stuff in /base. But then, for this, we would > > just > > > > add /win8/common/secondary_tile_types.h . > > > > > > > > Alternatively, [option "3"] I can get that ball rolling in this CL. That > is, > > > > start with /win8/common/{win8_common.gypi,secondary_tile_types.h}. And add > > > > OS=="win" ==> win8_common.gypi dependency to chrome_browser.gypi, right > > under > > > > '../ui/metro_viewer/metro_viewer.gyp:metro_viewer', > > > > then things can start moving out of /base/win in follow-ups. > > > > > > > > Or try out a CL for moving everything first [option "2"], and land it > before > > > > this (assuming it's feasible). > > > > > > > > An [option "4"]: > > > > * These enums are not necessarily UMA-specific: this comment could > rightly > > > drop > > > > the UMA wording and maybe feel less out of place. > > > > > > > > An [option "1.1"]: > > > > * Do "1", but put it in /win8/metro_uma > > > > > > > > And there's always ["option 0"]: no-op ;-). > > > > > > > > Nothing done yet -- WDYT? > > > > > > I was referring to all the secondary tile stuff you're adding, not just the > > UMA > > > stuff. Today I chatted with cpu@ about this and he also wasn't keen to put > > > secondary related tile stuff into this file. > > > > > > Why not just put it in win8/metro_driver/secondary_tile.h and refer to that > > from > > > metro_pin_tab_helper? > > > > ** Presubmit ERRORS ** > > You added one or more #includes that violate checkdeps rules. > > chrome\browser\ui\metro_pin_tab_helper_win.cc > > Illegal include: "win8/metro_driver/secondary_tile_types.h" > > Because of no rule applying. > > > > "One does not simply #include in Chrome..." ;) > > > > I've uploaded a patch with presubmit hooks skippped to make sure this is what > > you had in mind. I'll look into policies/procedures around this tomorrow. > > Perhaps, for approval, there is a "right" way to do it (maybe along the lines > of > > option [3]). > > If these are specific to UMA I would leave them here and put UMA in the name > somewhere... > > https://codereview.chromium.org/11280112/diff/5013/chrome/chrome_browser_ui.gypi > File chrome/chrome_browser_ui.gypi (right): > > https://codereview.chromium.org/11280112/diff/5013/chrome/chrome_browser_ui.g... > chrome/chrome_browser_ui.gypi:2658: > '../win8/metro_driver/secondary_tile_types.h', > We should not include stuff from the win8 folder here... > > https://codereview.chromium.org/11280112/diff/5013/win8/metro_driver/metro_dr... > File win8/metro_driver/metro_driver.gyp (right): > > https://codereview.chromium.org/11280112/diff/5013/win8/metro_driver/metro_dr... > win8/metro_driver/metro_driver.gyp:101: 'secondary_tile_types.h', > I don't see this file in your CL? > > https://codereview.chromium.org/11280112/diff/5013/win8/metro_driver/secondar... > File win8/metro_driver/secondary_tile.cc (right): > > https://codereview.chromium.org/11280112/diff/5013/win8/metro_driver/secondar... > win8/metro_driver/secondary_tile.cc:200: // Ensure function signature is > correct. > I don't understand this comment...
types have moved back to base/win/metro.h; named to include 'Uma' where appropriate. I've also made the callback arguments const-reference - some greps suggest that, on balance, that's more common (but of course the callback class data member is still copy constructed). PTAL. https://codereview.chromium.org/11280112/diff/4005/base/win/metro.h File base/win/metro.h (right): https://codereview.chromium.org/11280112/diff/4005/base/win/metro.h#newcode40 base/win/metro.h:40: // Enum values for UMA histogram reporting of site-specific tile pinning. On 2012/11/29 15:26:18, MAD wrote: > On 2012/11/29 07:19:27, tapted wrote: > > On 2012/11/29 04:19:46, benwells wrote: > > > On 2012/11/28 00:33:43, tapted wrote: > > > > On 2012/11/27 08:56:39, benwells wrote: > > > > > Does this stuff all have to go in this file? It doesn't feel right going > > in > > > > > base. > > > > > > > > It needs to go somewhere shared between /win8 and /chrome/browser/ui.. > > > > > > > > Currently, viable options (i.e. current viable dependencies of both) are: > > > > * /base, and > > > > * /chrome/common/common_constants, > > > > > > > > /base/win/metro.h currently seems to be the most common way for things to > be > > > > shared between the metro driver and the win8 parts of the browser. > > > > > > > > There is also: > > > > * /ui/metro_viewer/[metro_viewer_messages.h] > > > > > > > > An option [option "1"] in the same vein as this would be to make: > > > > * /ui/metro_uma/ > > > > * /ui/metro_uma/metro_uma.gyp > > > > * /ui/metro_uma/metro_uma_types.h > > > > > > > > That feels a bit heavyweight. > > > > > > > > The "right" solution to me feels like: > > > > * /win8/common/ > > > > * /win8/common/win8_common.gyp (or win8/win8_common.gyp .. or .gypi ) > > > > > > > > Then the stuff currently in /base/win/metro.h moves to > /win8/common/metro.h. > > > > That feels like a separate CL though [option "2"], and there might be a > good > > > > reason for having some metro stuff in /base. But then, for this, we would > > just > > > > add /win8/common/secondary_tile_types.h . > > > > > > > > Alternatively, [option "3"] I can get that ball rolling in this CL. That > is, > > > > start with /win8/common/{win8_common.gypi,secondary_tile_types.h}. And add > > > > OS=="win" ==> win8_common.gypi dependency to chrome_browser.gypi, right > > under > > > > '../ui/metro_viewer/metro_viewer.gyp:metro_viewer', > > > > then things can start moving out of /base/win in follow-ups. > > > > > > > > Or try out a CL for moving everything first [option "2"], and land it > before > > > > this (assuming it's feasible). > > > > > > > > An [option "4"]: > > > > * These enums are not necessarily UMA-specific: this comment could > rightly > > > drop > > > > the UMA wording and maybe feel less out of place. > > > > > > > > An [option "1.1"]: > > > > * Do "1", but put it in /win8/metro_uma > > > > > > > > And there's always ["option 0"]: no-op ;-). > > > > > > > > Nothing done yet -- WDYT? > > > > > > I was referring to all the secondary tile stuff you're adding, not just the > > UMA > > > stuff. Today I chatted with cpu@ about this and he also wasn't keen to put > > > secondary related tile stuff into this file. > > > > > > Why not just put it in win8/metro_driver/secondary_tile.h and refer to that > > from > > > metro_pin_tab_helper? > > > > ** Presubmit ERRORS ** > > You added one or more #includes that violate checkdeps rules. > > chrome\browser\ui\metro_pin_tab_helper_win.cc > > Illegal include: "win8/metro_driver/secondary_tile_types.h" > > Because of no rule applying. > > > > "One does not simply #include in Chrome..." ;) > > > > I've uploaded a patch with presubmit hooks skippped to make sure this is what > > you had in mind. I'll look into policies/procedures around this tomorrow. > > Perhaps, for approval, there is a "right" way to do it (maybe along the lines > of > > option [3]). > > If these are specific to UMA I would leave them here and put UMA in the name > somewhere... Done. https://codereview.chromium.org/11280112/diff/5013/chrome/chrome_browser_ui.gypi File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/11280112/diff/5013/chrome/chrome_browser_ui.g... chrome/chrome_browser_ui.gypi:2658: '../win8/metro_driver/secondary_tile_types.h', On 2012/11/29 15:26:18, MAD wrote: > We should not include stuff from the win8 folder here... Done. https://codereview.chromium.org/11280112/diff/5013/win8/metro_driver/metro_dr... File win8/metro_driver/metro_driver.gyp (right): https://codereview.chromium.org/11280112/diff/5013/win8/metro_driver/metro_dr... win8/metro_driver/metro_driver.gyp:101: 'secondary_tile_types.h', On 2012/11/29 15:26:18, MAD wrote: > I don't see this file in your CL? Drat silly mistake :-/ - the contents will move back to base/win/metro.h in the next patchset. https://codereview.chromium.org/11280112/diff/5013/win8/metro_driver/secondar... File win8/metro_driver/secondary_tile.cc (right): https://codereview.chromium.org/11280112/diff/5013/win8/metro_driver/secondar... win8/metro_driver/secondary_tile.cc:200: // Ensure function signature is correct. On 2012/11/29 15:26:18, MAD wrote: > I don't understand this comment... Sorry - thanks for catching it - that was a stray from an earlier patchset. Fixed.
https://codereview.chromium.org/11280112/diff/10010/win8/metro_driver/chrome_... File win8/metro_driver/chrome_app_view.cc (left): https://codereview.chromium.org/11280112/diff/10010/win8/metro_driver/chrome_... win8/metro_driver/chrome_app_view.cc:309: winfoundtn::IAsyncOperation<bool>* async, I like that this is gone from here :) https://codereview.chromium.org/11280112/diff/10010/win8/metro_driver/seconda... File win8/metro_driver/secondary_tile.cc (right): https://codereview.chromium.org/11280112/diff/10010/win8/metro_driver/seconda... win8/metro_driver/secondary_tile.cc:22: class TileRequestDone { Can you rename this class to TileRequestCompletedHandler? Or DoneHandler? Or even completion? Some sort noun. https://codereview.chromium.org/11280112/diff/10010/win8/metro_driver/seconda... win8/metro_driver/secondary_tile.cc:182: (new TileRequestDone(TileRequestDone::PIN, Since this is over two lines anyway, can you introduce a variable?
https://codereview.chromium.org/11280112/diff/10010/win8/metro_driver/seconda... File win8/metro_driver/secondary_tile.cc (right): https://codereview.chromium.org/11280112/diff/10010/win8/metro_driver/seconda... win8/metro_driver/secondary_tile.cc:22: class TileRequestDone { On 2012/11/30 01:16:47, benwells wrote: > Can you rename this class to TileRequestCompletedHandler? Or DoneHandler? Or > even completion? Some sort noun. Done. Went for TileRequestCompleter .. since it `Complete`s the `completion` further down. https://codereview.chromium.org/11280112/diff/10010/win8/metro_driver/seconda... win8/metro_driver/secondary_tile.cc:182: (new TileRequestDone(TileRequestDone::PIN, On 2012/11/30 01:16:47, benwells wrote: > Since this is over two lines anyway, can you introduce a variable? Done.
lgtm
LGTM with one small request... Thanks! BYE MAD https://codereview.chromium.org/11280112/diff/12013/win8/metro_driver/seconda... File win8/metro_driver/secondary_tile.cc (right): https://codereview.chromium.org/11280112/diff/12013/win8/metro_driver/seconda... win8/metro_driver/secondary_tile.cc:34: HRESULT Respond(winfoundtn::IAsyncOperation<bool>* async, Please add a comment that this will destroy the object.
Check out this CL which is introducing win8/util, which /content and /printing will depend on. Could the shared pinning stuff move there too? https://codereview.chromium.org/11411286/
On 2012/11/30 18:56:31, benwells wrote: > Check out this CL which is introducing win8/util, which /content and /printing > will depend on. Could the shared pinning stuff move there too? > > https://codereview.chromium.org/11411286/ I think so - it looks like that's the intended direction. From the changelog: > Also introduces win8_util where some other win8-specific things currently in base should eventually also be refactored into. chrome/browser/DEPS is also updating there to allow #includes from win8/util, which is the one metro_pin_tab_helper_win.cc would want. However, it also uses base::win::GetMetroModule() which might not be moving util a follow-up CL. If there's a preference to keep GetMetroModule() and the types around functions dynamically loaded from it together, we might still want to go with base/win for this CL. Otherwise, we can wait and hope that cl11411286 lands quickly. Thoughts? https://codereview.chromium.org/11280112/diff/12013/win8/metro_driver/seconda... File win8/metro_driver/secondary_tile.cc (right): https://codereview.chromium.org/11280112/diff/12013/win8/metro_driver/seconda... win8/metro_driver/secondary_tile.cc:34: HRESULT Respond(winfoundtn::IAsyncOperation<bool>* async, On 2012/11/30 18:18:37, MAD wrote: > Please add a comment that this will destroy the object. Done.
On 2012/12/03 02:58:15, tapted wrote: > On 2012/11/30 18:56:31, benwells wrote: > > Check out this CL which is introducing win8/util, which /content and /printing > > will depend on. Could the shared pinning stuff move there too? > > > > https://codereview.chromium.org/11411286/ > > I think so - it looks like that's the intended direction. From the changelog: > > Also introduces win8_util where some other win8-specific things currently in > base should eventually also be refactored into. > > chrome/browser/DEPS is also updating there to allow #includes from win8/util, > which is the one metro_pin_tab_helper_win.cc would want. However, it also uses > base::win::GetMetroModule() which might not be moving util a follow-up CL. > > If there's a preference to keep GetMetroModule() and the types around functions > dynamically loaded from it together, we might still want to go with base/win for > this CL. Otherwise, we can wait and hope that cl11411286 lands quickly. > Thoughts? Let's put this to base owners, if a new folder comes along later we'll move it then. > > https://codereview.chromium.org/11280112/diff/12013/win8/metro_driver/seconda... > File win8/metro_driver/secondary_tile.cc (right): > > https://codereview.chromium.org/11280112/diff/12013/win8/metro_driver/seconda... > win8/metro_driver/secondary_tile.cc:34: HRESULT > Respond(winfoundtn::IAsyncOperation<bool>* async, > On 2012/11/30 18:18:37, MAD wrote: > > Please add a comment that this will destroy the object. > > Done.
+cpu,sky could you please take a look for OWNERS sky for chrome/browser/ui https://codereview.chromium.org/11280112/diff/8019/chrome/browser/ui/metro_pi... cpu for base/win https://codereview.chromium.org/11280112/diff/8019/base/win/metro.h (and comment below) On 2012/12/03 06:01:27, benwells wrote: > On 2012/12/03 02:58:15, tapted wrote: > > On 2012/11/30 18:56:31, benwells wrote: > > > Check out this CL which is introducing win8/util, which /content and > /printing > > > will depend on. Could the shared pinning stuff move there too? > > > > > > https://codereview.chromium.org/11411286/ > > > > I think so - it looks like that's the intended direction. From the changelog: > > > Also introduces win8_util where some other win8-specific things currently in > > base should eventually also be refactored into. > > > > chrome/browser/DEPS is also updating there to allow #includes from win8/util, > > which is the one metro_pin_tab_helper_win.cc would want. However, it also uses > > base::win::GetMetroModule() which might not be moving util a follow-up CL. > > > > If there's a preference to keep GetMetroModule() and the types around > functions > > dynamically loaded from it together, we might still want to go with base/win > for > > this CL. Otherwise, we can wait and hope that cl11411286 lands quickly. > > Thoughts? > Let's put this to base owners, if a new folder comes along later we'll move it > then.
LGTM
lgtm https://codereview.chromium.org/11280112/diff/8019/base/win/metro.h File base/win/metro.h (right): https://codereview.chromium.org/11280112/diff/8019/base/win/metro.h#newcode41 base/win/metro.h:41: // Enum values for UMA histogram reporting of site-specific tile pinning. please create a todo and a bug to remove this from here in some future date.
https://codereview.chromium.org/11280112/diff/8019/base/win/metro.h File base/win/metro.h (right): https://codereview.chromium.org/11280112/diff/8019/base/win/metro.h#newcode41 base/win/metro.h:41: // Enum values for UMA histogram reporting of site-specific tile pinning. On 2012/12/04 00:46:50, cpu wrote: > please create a todo and a bug to remove this from here in some future date. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tapted@chromium.org/11280112/6020
Retried try job too often on mac_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tapted@chromium.org/11280112/6020
Message was sent while issue was closed.
Change committed as 170916 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
