Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2912473004:
[MD settings] rename lastRoutChangeWasPopstate var (Closed)
|
Created:
3 years, 6 months ago by dschuyler Modified:
3 years, 6 months ago Reviewers:
dpapad CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] rename lastRoutChangeWasPopstate var
Changing var name to address presubmit requirement.
(code health)
BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2912473004
Cr-Commit-Position: refs/heads/master@{#475644}
Committed: https://chromium.googlesource.com/chromium/src/+/8d702339698fb0f317fcc4e594d91007c47b25f0
Patch Set 1 #Patch Set 2 : name change #
Total comments: 5
Patch Set 3 : swapped names #
Total comments: 4
Patch Set 4 : updated test #
Messages
Total messages: 27 (17 generated)
Description was changed from ========== [MD settings] rename lastRoutChangeWasPopstate var Changing var name to address presubmit requirement. (code health) BUG=None ========== to ========== [MD settings] rename lastRoutChangeWasPopstate var Changing var name to address presubmit requirement. (code health) BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@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 dschuyler@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...
dschuyler@chromium.org changed reviewers: + dpapad@chromium.org
There's an unrelated change I want to make and hit a presubmit error with lastRoutChangeWasPopstate_ having an underscore. This CL is to address that issue separately. There are several ways to address the issue - feel free to suggest your favorite :) https://codereview.chromium.org/2912473004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2912473004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/route.js:367: }; I'm tempted to remove this function entirely and allow access to the variable. WDYT?
https://codereview.chromium.org/2912473004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2912473004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/route.js:365: var wasLastRouteChangePopstate = function() { My understanding is that you are forced to rename this method, otherwise it will collide with the new name of of the boolean variable. If that's accurate, how about going the reverse direction? lastRouteChangeWasPopstate_ -> wasLastRouteChangePopstate lastRouteChangeWasPopstate -> No need to change, stays the same This would produces a smaller diff, which is the only way I am suggesting it. https://codereview.chromium.org/2912473004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/route.js:367: }; On 2017/05/26 at 23:29:59, dschuyler wrote: > I'm tempted to remove this function entirely and allow access to the variable. WDYT? It seems that this would break the intention of the existing code, which is to allow read access, but not write access.
On 2017/05/27 at 00:25:57, dpapad wrote: > https://codereview.chromium.org/2912473004/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/route.js (right): > > https://codereview.chromium.org/2912473004/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/route.js:365: var wasLastRouteChangePopstate = function() { > My understanding is that you are forced to rename this method, otherwise it will collide with the new name of of the boolean variable. > > If that's accurate, how about going the reverse direction? > > lastRouteChangeWasPopstate_ -> wasLastRouteChangePopstate > lastRouteChangeWasPopstate -> No need to change, stays the same > > This would produces a smaller diff, which is the only way I am suggesting it. *which is why I am suggesting it > > https://codereview.chromium.org/2912473004/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/route.js:367: }; > On 2017/05/26 at 23:29:59, dschuyler wrote: > > I'm tempted to remove this function entirely and allow access to the variable. WDYT? > > It seems that this would break the intention of the existing code, which is to allow read access, but not write access.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dschuyler@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...
Is this what you were suggesting? https://codereview.chromium.org/2912473004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2912473004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/route.js:365: var wasLastRouteChangePopstate = function() { On 2017/05/27 00:25:56, dpapad wrote: > My understanding is that you are forced to rename this method, otherwise it will > collide with the new name of of the boolean variable. > > If that's accurate, how about going the reverse direction? > > lastRouteChangeWasPopstate_ -> wasLastRouteChangePopstate > lastRouteChangeWasPopstate -> No need to change, stays the same > > This would produces a smaller diff, which is the only way I am suggesting it. Done. https://codereview.chromium.org/2912473004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/route.js:367: }; On 2017/05/27 00:25:56, dpapad wrote: > On 2017/05/26 at 23:29:59, dschuyler wrote: > > I'm tempted to remove this function entirely and allow access to the variable. > WDYT? > > It seems that this would break the intention of the existing code, which is to > allow read access, but not write access. Acknowledged.
Yes, this is what I was suggesting. One optional nit, and one comment. https://codereview.chromium.org/2912473004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2912473004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/main_page_behavior.js:16: * the vertical expand animation). Assuming you cleaned up this file, since it was already being touched by this CL at patch#2. This is no longer the case in patch#3 though. Optional: Consider reverting this file from this CL. https://codereview.chromium.org/2912473004/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/route_tests.js (right): https://codereview.chromium.org/2912473004/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/route_tests.js:154: assertFalse(settings.wasLastRouteChangePopstate()); This file should be reverted too, since this function did not change name as of patch#3.
Yes, this is what I was suggesting. One optional nit, and one comment.
The CQ bit was checked by dschuyler@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/2912473004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2912473004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/main_page_behavior.js:16: * the vertical expand animation). On 2017/05/27 01:29:51, dpapad wrote: > Assuming you cleaned up this file, since it was already being touched by this CL > at patch#2. This is no longer the case in patch#3 though. Optional: Consider > reverting this file from this CL. I reflected on that, but decided these were worthwhile changes. https://codereview.chromium.org/2912473004/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/route_tests.js (right): https://codereview.chromium.org/2912473004/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/route_tests.js:154: assertFalse(settings.wasLastRouteChangePopstate()); On 2017/05/27 01:29:51, dpapad wrote: > This file should be reverted too, since this function did not change name as of > patch#3. Thanks for catching this!
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dschuyler@chromium.org
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": 60001, "attempt_start_ts": 1496168245565970,
"parent_rev": "b244ba193727b07b030b16dd678d131c461d0712", "commit_rev":
"8d702339698fb0f317fcc4e594d91007c47b25f0"}
Message was sent while issue was closed.
Description was changed from ========== [MD settings] rename lastRoutChangeWasPopstate var Changing var name to address presubmit requirement. (code health) BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] rename lastRoutChangeWasPopstate var Changing var name to address presubmit requirement. (code health) BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2912473004 Cr-Commit-Position: refs/heads/master@{#475644} Committed: https://chromium.googlesource.com/chromium/src/+/8d702339698fb0f317fcc4e594d9... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/8d702339698fb0f317fcc4e594d9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
