Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2160713002:
MD Settings: refactor some main page properties, fix Advanced toggle (Closed)
|
Created:
4 years, 5 months ago by michaelpg Modified:
4 years, 5 months ago Reviewers:
dschuyler CC:
dpapad, arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, Dan Beam, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: refactor some main page properties, fix Advanced toggle
Fixes the Advanced toggle when navigating directly to Advanced.
Make currentRouteChanged_ less confusing. Wrap non-About stuff in a dom-if.
Removes a bunch of redundant properties.
Part of https://codereview.chromium.org/2106013002/
BUG=630010, 589681
R=dbeam@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/5b7b3f37f385179dac9a1ae4407a20fb2a28647a
Cr-Commit-Position: refs/heads/master@{#407236}
Patch Set 1 #Patch Set 2 #
Total comments: 1
Patch Set 3 : not computed #Patch Set 4 : no default values #Patch Set 5 : . #
Total comments: 2
Patch Set 6 : object #
Total comments: 2
Patch Set 7 : lowercase #
Total comments: 10
Patch Set 8 : Feedback #Patch Set 9 : return to dom-if why not #
Messages
Total messages: 48 (22 generated)
Description was changed from ========== MD Settings: computed properties for main page visibility BUG=589681 R=dbeam@chromium.org ========== to ========== MD Settings: computed properties for main page visibility BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: computed properties for main page visibility BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: computed properties for main page visibility Make currentRouteChanged_ less confusing by moving logic into computed properties. Part of https://codereview.chromium.org/2106013002/ BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
michaelpg@chromium.org changed reviewers: + dpapad@chromium.org
dpapad: I wanted to make these computed properties because currentRouteChanged_ was getting more and more complicated. I guess that's not a good idea now, if you want to be able to pull these levers arbitrarily. Should I throw this out? https://codereview.chromium.org/2160713002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2160713002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:147: this.showBasicPage_ = true; damn. I guess I can't make these computed properties then?
On 2016/07/19 at 23:55:35, michaelpg wrote: > dpapad: I wanted to make these computed properties because currentRouteChanged_ was getting more and more complicated. I guess that's not a good idea now, if you want to be able to pull these levers arbitrarily. Should I throw this out? > > https://codereview.chromium.org/2160713002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/settings_main/settings_main.js (right): > > https://codereview.chromium.org/2160713002/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/settings_main/settings_main.js:147: this.showBasicPage_ = true; > damn. I guess I can't make these computed properties then? You are right. These need to be manually set to true when a search is being issued, which seems conflicting with making them computed properties. Perhaps you could split them to two booleans per basic/advanced, one computed and one used by search to "force" render those, but I am not sure why would that be worth it.
On 2016/07/20 00:02:21, dpapad wrote: > On 2016/07/19 at 23:55:35, michaelpg wrote: > > dpapad: I wanted to make these computed properties because > currentRouteChanged_ was getting more and more complicated. I guess that's not a > good idea now, if you want to be able to pull these levers arbitrarily. Should I > throw this out? > > > > > https://codereview.chromium.org/2160713002/diff/20001/chrome/browser/resource... > > File chrome/browser/resources/settings/settings_main/settings_main.js (right): > > > > > https://codereview.chromium.org/2160713002/diff/20001/chrome/browser/resource... > > chrome/browser/resources/settings/settings_main/settings_main.js:147: > this.showBasicPage_ = true; > > damn. I guess I can't make these computed properties then? > > You are right. These need to be manually set to true when a search is being > issued, which seems conflicting with making them computed properties. > Perhaps you could split them to two booleans per basic/advanced, one computed > and one used by search to "force" render those, but I am not sure why would > that be worth it. OK, thanks. I think we can still simplify it by switching to if/else.. uploading soon.
Description was changed from ========== MD Settings: computed properties for main page visibility Make currentRouteChanged_ less confusing by moving logic into computed properties. Part of https://codereview.chromium.org/2106013002/ BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: refactor some main page properties Make currentRouteChanged_ less confusing. Wrap non-About stuff in a !showAboutPage_ dom-if. Remove redundant properties isAdvancedMenuOpen_, showAdvancedToggle_. Part of https://codereview.chromium.org/2106013002/ BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
FWIW, this does not seem to affect issue 629696 or issue 629697. However, dpapad@ it would be nice if you can confirm these are sane changes to make. Motivation is these properties are nicer for transitions (https://codereview.chromium.org/2106013002) and IMO saner.
michaelpg@chromium.org changed reviewers: - dbeam@chromium.org, dpapad@chromium.org
actually hold off for a bit, please
Patchset #4 (id:60001) has been deleted
Description was changed from ========== MD Settings: refactor some main page properties Make currentRouteChanged_ less confusing. Wrap non-About stuff in a !showAboutPage_ dom-if. Remove redundant properties isAdvancedMenuOpen_, showAdvancedToggle_. Part of https://codereview.chromium.org/2106013002/ BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: refactor some main page properties Make currentRouteChanged_ less confusing. Wrap non-About stuff in a !showAboutPage_ dom-if. Remove redundant properties isAdvancedMenuOpen_, showAdvancedToggle_. Remove default values of other props. Part of https://codereview.chromium.org/2106013002/ BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
michaelpg@chromium.org changed reviewers: + dbeam@chromium.org
OK, ready to review. PTAL.
dbeam@chromium.org changed reviewers: + dschuyler@chromium.org
I think dschuyler@ is a better reviewer for this
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
dpapad@chromium.org changed reviewers: + dbeam@chromium.org, dpapad@chromium.org - dschuyler@chromium.org
https://codereview.chromium.org/2160713002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2160713002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:116: this.showAboutPage_ = newRoute.page == 'about'; Drive-by question: Would it make more sense to package the three booleans to a single Polymer property? showingPages_: { type: Boolean, value: {basic: false, advanced: false, about: false}, } This way you can replace all 3 booleans at once, instead of flipping them one by one. Maybe that invokes less DOM updates?
dpapad@chromium.org changed reviewers: - dpapad@chromium.org
dpapad@chromium.org changed reviewers: + dschuyler@chromium.org - dbeam@chromium.org
Description was changed from ========== MD Settings: refactor some main page properties Make currentRouteChanged_ less confusing. Wrap non-About stuff in a !showAboutPage_ dom-if. Remove redundant properties isAdvancedMenuOpen_, showAdvancedToggle_. Remove default values of other props. Part of https://codereview.chromium.org/2106013002/ BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: refactor some main page properties, fix Advanced toggle Fixes the Advanced toggle when navigating directly to Advanced. Make currentRouteChanged_ less confusing. Wrap non-About stuff in a dom-if. Removes a bunch of redundant properties. Part of https://codereview.chromium.org/2106013002/ BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dschuyler: PTAL. https://codereview.chromium.org/2160713002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2160713002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:116: this.showAboutPage_ = newRoute.page == 'about'; On 2016/07/20 21:30:08, dpapad wrote: > Drive-by question: Would it make more sense to package the three booleans to a > single Polymer property? > > showingPages_: { > type: Boolean, > value: {basic: false, advanced: false, about: false}, > } > > This way you can replace all 3 booleans at once, instead of flipping them one by > one. Maybe that invokes less DOM updates? Fine idea, done.
https://codereview.chromium.org/2160713002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2160713002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.html:41: <template is="dom-if" if="[[!showPages_.ABOUT]]"> Why is about all caps, as if it were a const?
Is there a more specific bug for these changes other than BUG=589681
Description was changed from ========== MD Settings: refactor some main page properties, fix Advanced toggle Fixes the Advanced toggle when navigating directly to Advanced. Make currentRouteChanged_ less confusing. Wrap non-About stuff in a dom-if. Removes a bunch of redundant properties. Part of https://codereview.chromium.org/2106013002/ BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: refactor some main page properties, fix Advanced toggle Fixes the Advanced toggle when navigating directly to Advanced. Make currentRouteChanged_ less confusing. Wrap non-About stuff in a dom-if. Removes a bunch of redundant properties. Part of https://codereview.chromium.org/2106013002/ BUG=630010,589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/07/21 02:46:11, dschuyler wrote: > Is there a more specific bug for these changes other than BUG=589681 added crbug.com/630010
https://codereview.chromium.org/2160713002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2160713002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.html:41: <template is="dom-if" if="[[!showPages_.ABOUT]]"> On 2016/07/21 02:38:22, dschuyler wrote: > Why is about all caps, as if it were a const? Hmm, I guess it's not a const (since yeah, it's a bool that changes). It's the keys that are constant. So you're right, showPages_ is not an @enum, but it's not a @dict or @unrestricted either (which is what most of our Object properties really are) -- the keys aren't arbitrary. Maybe @struct?
back in my day we had bitmasks and << and | and & and ~ and we liked it! (p.s. I am not actually recommending a bitmask, but do agree with dschuyler that CONST_CASE_TO_SCARE_SMALL_CHILDREN probably isn't correct here)
https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.html:41: <template is="dom-if" if="[[!showPages_.about]]"> Consider adding a comment about why there is an extra level of dom-if is here. Since it's not strictly necessary, is it an optimization or just preference - if it's an optimization, is it optimizing for the 'basic' settings page (the expected common case)? https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:122: this.advancedToggleExpanded_ = true; When would this be needed (when would it not be set by the toggle-advanced-page event)? If we can, I'd like for this to be set in one place rather than two. It looks like a toggle-advanced-page event may cause this to be set twice (once for the event and once for the current route changed). https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:152: this.set('showPages_.basic', true); With the new struct to set the basic, advanced, about settings all at once; would it be better to set them all at once here as well? What is the advantage of setting them one at a time?
https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.html:41: <template is="dom-if" if="[[!showPages_.about]]"> On 2016/07/21 19:02:05, dschuyler wrote: > Consider adding a comment about why there is > an extra level of dom-if is here. Since it's not > strictly necessary, is it an optimization or > just preference - if it's an optimization, is > it optimizing for the 'basic' settings page > (the expected common case)? it's just for simplicity. These three elements (basic page, advanced toggle, advanced page) are logically grouped: they can all be shown at the same time, and they can't be mixed with the About page. The old code has them all at the top level alongside About; the new code is a tree with About and non-About in separate subtrees. Without the dom-if we need to add back "showAdvancedToggle_" and keep track of that separately, or add a function showAdvancedToggle_(inSubpage_, showPages_). That seems redundant to me, but maybe it comes down to preference. https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:122: this.advancedToggleExpanded_ = true; On 2016/07/21 19:02:05, dschuyler wrote: > When would this be needed (when would it not > be set by the toggle-advanced-page event)? When you navigate to chrome://md-settings/advanced. Either as the first page you visit, or using back/forward, the toggle needs to flip on its own sometimes. So, neither the route nor the click handler has all necessary information to determine this property in one place. (Thus the bug.) You could argue the route should include this info, but IMO that state should belong to settings-main as it isn't necessarily tied to navigation. > If > we can, I'd like for this to be set in one > place rather than two. Why? It's not a complicated member or set to a complicated value. > It looks like a > toggle-advanced-page event may cause this to be > set twice (once for the event and once for the > current route changed). Maybe the answer is that the user toggling whether the advanced page is shown should not be considered a navigation event (shouldn't affect the route). https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:152: this.set('showPages_.basic', true); On 2016/07/21 19:02:05, dschuyler wrote: > With the new struct to set the basic, advanced, about > settings all at once; would it be better to set them > all at once here as well? What is the advantage of > setting them one at a time? You'd have to ask dpapad whether it matters. I don't really want to mess with this logic right now.
https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:152: this.set('showPages_.basic', true); On 2016/07/21 at 19:55:28, michaelpg wrote: > On 2016/07/21 19:02:05, dschuyler wrote: > > With the new struct to set the basic, advanced, about > > settings all at once; would it be better to set them > > all at once here as well? What is the advantage of > > setting them one at a time? > > You'd have to ask dpapad whether it matters. I don't really want to mess with this logic right now. I believe that setting both booleans at once here would be equivalent. Let me clarify a bit why I suggested packaging the three booleans in a single object in the first place. Mostly because of readability. Packaging visibility booleans in an object is a pattern I have encountered in other Polymer code, for example it is also being done at https://codereview.chromium.org/2106103006 (see pageVisibility object). It is also something I wanted to do as a cleanup at https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/certif.... The performance aspect of assigning all booleans at once, is secondary and only applicable if the previous version of the code was flipping booleans twice (where the first value was basically thrown away). Looking back at the previous patch were I suggested packaging the booleans, I don't think there was any flipping going on. I still think this is cleaner, but not a big deal, feel free to unpackage the booleans if you don't think is more readable, I am OK with either way.
Patchset #9 (id:180001) has been deleted
Patchset #9 (id:200001) has been deleted
Please take another [hopefully last] look! https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.html:41: <template is="dom-if" if="[[!showPages_.about]]"> On 2016/07/21 19:55:28, michaelpg wrote: > On 2016/07/21 19:02:05, dschuyler wrote: > > Consider adding a comment about why there is > > an extra level of dom-if is here. Since it's not > > strictly necessary, is it an optimization or > > just preference - if it's an optimization, is > > it optimizing for the 'basic' settings page > > (the expected common case)? > > it's just for simplicity. These three elements (basic page, advanced toggle, > advanced page) are logically grouped: they can all be shown at the same time, > and they can't be mixed with the About page. The old code has them all at the > top level alongside About; the new code is a tree with About and non-About in > separate subtrees. > > Without the dom-if we need to add back "showAdvancedToggle_" and keep track of > that separately, or add a function showAdvancedToggle_(inSubpage_, showPages_). > That seems redundant to me, but maybe it comes down to preference. In the interest of making rebasing easier and this CL smaller I've reverted these changes. Using a computed binding instead (showAdvancedToggle_). https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:122: this.advancedToggleExpanded_ = true; On 2016/07/21 19:55:28, michaelpg wrote: > On 2016/07/21 19:02:05, dschuyler wrote: > > When would this be needed (when would it not > > be set by the toggle-advanced-page event)? > > When you navigate to chrome://md-settings/advanced. Either as the first page you > visit, or using back/forward, the toggle needs to flip on its own sometimes. > > So, neither the route nor the click handler has all necessary information to > determine this property in one place. (Thus the bug.) > > You could argue the route should include this info, but IMO that state should > belong to settings-main as it isn't necessarily tied to navigation. > > > If > > we can, I'd like for this to be set in one > > place rather than two. > > Why? It's not a complicated member or set to a complicated value. > > > It looks like a > > toggle-advanced-page event may cause this to be > > set twice (once for the event and once for the > > current route changed). > > Maybe the answer is that the user toggling whether the advanced page is shown > should not be considered a navigation event (shouldn't affect the route). So this definitely needs to change to fix Advanced bugs, but this CL is probably not the right place to do it, and we should clarify the expected behavior first. Sound OK? https://codereview.chromium.org/2160713002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:152: this.set('showPages_.basic', true); On 2016/07/21 22:31:27, dpapad wrote: > On 2016/07/21 at 19:55:28, michaelpg wrote: > > On 2016/07/21 19:02:05, dschuyler wrote: > > > With the new struct to set the basic, advanced, about > > > settings all at once; would it be better to set them > > > all at once here as well? What is the advantage of > > > setting them one at a time? > > > > You'd have to ask dpapad whether it matters. I don't really want to mess with > this logic right now. > > I believe that setting both booleans at once here would be equivalent. > > Let me clarify a bit why I suggested packaging the three booleans in a single > object in the first place. Mostly because of readability. Packaging visibility > booleans in an object is a pattern I have encountered in other Polymer code, for > example it is also being done at https://codereview.chromium.org/2106103006 (see > pageVisibility object). It is also something I wanted to do as a cleanup at > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/certif.... > > The performance aspect of assigning all booleans at once, is secondary and only > applicable if the previous version of the code was flipping booleans twice > (where the first value was basically thrown away). Looking back at the previous > patch were I suggested packaging the booleans, I don't think there was any > flipping going on. I still think this is cleaner, but not a big deal, feel free > to unpackage the booleans if you don't think is more readable, I am OK with > either way. Nah, the struct of booleans is good step forward. Anyway, we'll improve this as we continue making changes to routing/navigation so it's not set in stone.
lgtm
The CQ bit was checked by michaelpg@chromium.org
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by michaelpg@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 ========== MD Settings: refactor some main page properties, fix Advanced toggle Fixes the Advanced toggle when navigating directly to Advanced. Make currentRouteChanged_ less confusing. Wrap non-About stuff in a dom-if. Removes a bunch of redundant properties. Part of https://codereview.chromium.org/2106013002/ BUG=630010,589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: refactor some main page properties, fix Advanced toggle Fixes the Advanced toggle when navigating directly to Advanced. Make currentRouteChanged_ less confusing. Wrap non-About stuff in a dom-if. Removes a bunch of redundant properties. Part of https://codereview.chromium.org/2106013002/ BUG=630010,589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: refactor some main page properties, fix Advanced toggle Fixes the Advanced toggle when navigating directly to Advanced. Make currentRouteChanged_ less confusing. Wrap non-About stuff in a dom-if. Removes a bunch of redundant properties. Part of https://codereview.chromium.org/2106013002/ BUG=630010,589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: refactor some main page properties, fix Advanced toggle Fixes the Advanced toggle when navigating directly to Advanced. Make currentRouteChanged_ less confusing. Wrap non-About stuff in a dom-if. Removes a bunch of redundant properties. Part of https://codereview.chromium.org/2106013002/ BUG=630010,589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/5b7b3f37f385179dac9a1ae4407a20fb2a28647a Cr-Commit-Position: refs/heads/master@{#407236} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/5b7b3f37f385179dac9a1ae4407a20fb2a28647a Cr-Commit-Position: refs/heads/master@{#407236} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
