Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1905493002:
Make the extension's sidebar pane auto-resizable. (Closed)
|
Created:
4 years, 8 months ago by kdzwinel Modified:
3 years, 10 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, devtools-reviews_chromium.org, extensions-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman, sergeyv+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake the extension's sidebar pane auto-resizable.
Setting height of the sidebar pane is a relic from the old design where panes were expandable (https://developer.chrome.com/static/images/devtools-panels.png). In the current design, where panes are tabs, it doesn't make sense.
BUG=438126
Committed: https://crrev.com/7ff544f5fa1d09072def5419d74f8c0e5f42365e
Cr-Commit-Position: refs/heads/master@{#391965}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Nuke ExtensionSidebarPane.setHeight completely. #Patch Set 3 : Nuke ExtensionSidebarPane.setHeight completely. #
Total comments: 3
Patch Set 4 : clean up defineDeprecatedMethod, update defineDeprecatedProperty accordingly #
Total comments: 3
Patch Set 5 : revert changes to defineDeprecatedProperty, fix indentation #Patch Set 6 : Add myself to the authors #Patch Set 7 : Fix layout tests #
Messages
Total messages: 47 (19 generated)
Description was changed from ========== Make the extension's sidebar pane auto-resizable. Deprecate setting fixed height of the sidebar pane, take the whole available space by default. BUG=438126 ========== to ========== Make the extension's sidebar pane auto-resizable. Setting height of the sidebar pane is a relic from the old design where panes were expandable (https://developer.chrome.com/static/images/devtools-panels.png). In the current design, where panes are tabs, it doesn't make sense. BUG=438126 ==========
kdzwinel@gmail.com changed reviewers: + caseq@chromium.org, paulirish@chromium.com, pfeldman@chromium.com
This bug affects my extensions and since it has a low priority I decided to give it a shoot myself. The tricky part is that one of the methods (ExtensionSidebarPane.setHeight) has to be deprecated and I have no idea what's your usual procedure in such cases. PTAL This is how I feel about this patch: http://i.imgur.com/w1f98zW.gif
https://codereview.chromium.org/1905493002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js (right): https://codereview.chromium.org/1905493002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:344: console.warn("Setting height of the SidebarPane is deprecated."); Let's move that to front-end API instead, so the server-side method can be nuked altogether -- we already have some precedents there: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2016/04/20 at 01:20:50, caseq wrote: > https://codereview.chromium.org/1905493002/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js (right): > > https://codereview.chromium.org/1905493002/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:344: console.warn("Setting height of the SidebarPane is deprecated."); > Let's move that to front-end API instead, so the server-side method can be nuked altogether -- we already have some precedents there: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Since `ExtensionSidebarPane.setHeight` is a method and `defineDeprecatedProperty` was meant for properties I created `defineDeprecatedMethod` and got rid of all `setHeight` references. Please take a look.
https://codereview.chromium.org/1905493002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js (right): https://codereview.chromium.org/1905493002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:395: if (!warningGiven) { nit: if (warningGiven) return; (we prefer early bail-out) https://codereview.chromium.org/1905493002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:399: return null; drop return altogether? https://codereview.chromium.org/1905493002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:401: object.oldName = noop.bind(null); Hmm... Does this work?
On 2016/04/20 at 18:17:12, caseq wrote: > https://codereview.chromium.org/1905493002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js (right): > > https://codereview.chromium.org/1905493002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:395: if (!warningGiven) { > nit: > if (warningGiven) > return; > > (we prefer early bail-out) > > https://codereview.chromium.org/1905493002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:399: return null; > drop return altogether? > > https://codereview.chromium.org/1905493002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:401: object.oldName = noop.bind(null); > Hmm... Does this work? All done. Besides `defineDeprecatedMethod` changes, I've also updated `defineDeprecatedProperty` with an early bail-out version of the warningGiven check.
https://codereview.chromium.org/1905493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js (right): https://codereview.chromium.org/1905493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:382: return; please revert this -- this is a different case, we are supposed to return the new property even after we issued the warning. https://codereview.chromium.org/1905493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:398: return; indent 4 spaces, please. https://codereview.chromium.org/1905493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:403: object[oldName] = noop.bind(null); we don't use this in noop, so I guess you can just remove bind here.
On 2016/04/20 at 23:00:47, caseq wrote: > https://codereview.chromium.org/1905493002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js (right): > > https://codereview.chromium.org/1905493002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:382: return; > please revert this -- this is a different case, we are supposed to return the new property even after we issued the warning. > > https://codereview.chromium.org/1905493002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:398: return; > indent 4 spaces, please. > > https://codereview.chromium.org/1905493002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:403: object[oldName] = noop.bind(null); > we don't use this in noop, so I guess you can just remove bind here. Done. I've set up editorconfig locally for this project to avoid indentation issues in the future.
The CQ bit was checked by caseq@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905493002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905493002/80001
The CQ bit was unchecked by caseq@chromium.org
The CQ bit was checked by kdzwinel@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org Link to the patchset: https://codereview.chromium.org/1905493002/#ps100001 (title: "Add myself to the authors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905493002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905493002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kdzwinel@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905493002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905493002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by caseq@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905493002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905493002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Finally a useful failure: https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel... Looks like some tests to adjust.
On 2016/04/21 at 03:25:48, paulirish wrote: > Finally a useful failure: > > https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel... > > Looks like some tests to adjust. Yeah that's definitely my change breaking the test. So, should I: - remove the test (since method it's testing is now deprecated) - test if panel takes up all available space (but this will be essentially testing if 'fill' CSS class does it's job) - change the test to check if `setHeight` is in fact a noop and does not modify the panel size ?
On 2016/04/21 11:26:39, kdzwinel wrote: > On 2016/04/21 at 03:25:48, paulirish wrote: > > Finally a useful failure: > > > > > https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel... > > > > Looks like some tests to adjust. > > Yeah that's definitely my change breaking the test. So, should I: > - remove the test (since method it's testing is now deprecated) > - test if panel takes up all available space (but this will be essentially > testing if 'fill' CSS class does it's job) > - change the test to check if `setHeight` is in fact a noop and does not modify > the panel size > ? The second option, testing that the "fill" is there and does the job looks best to me -- it may sound silly, but we've had plenty of problems in the past with things having been broken in a very similar situation of the ExtensionPanel, so you could perhaps borrow the test logic from there (it's a bit hairy, though): https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... But if it gets overly complicated, you can just remove dumping height from the test.
The CQ bit was checked by kdzwinel@gmail.com
The CQ bit was unchecked by kdzwinel@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905493002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905493002/100001
On 2016/04/21 at 21:40:16, caseq wrote: > On 2016/04/21 11:26:39, kdzwinel wrote: > > On 2016/04/21 at 03:25:48, paulirish wrote: > > > Finally a useful failure: > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel... > > > > > > Looks like some tests to adjust. > > > > Yeah that's definitely my change breaking the test. So, should I: > > - remove the test (since method it's testing is now deprecated) > > - test if panel takes up all available space (but this will be essentially > > testing if 'fill' CSS class does it's job) > > - change the test to check if `setHeight` is in fact a noop and does not modify > > the panel size > > ? > > The second option, testing that the "fill" is there and does the job looks best to me -- it may sound silly, but we've had plenty of problems in the past with things having been broken in a very similar situation of the ExtensionPanel, so you could perhaps borrow the test logic from there (it's a bit hairy, though): > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > But if it gets overly complicated, you can just remove dumping height from the test. Layout tests fixed. I borrowed the logic from extensions-panel as suggested. PTAL
Patchset #8 (id:140001) has been deleted
The CQ bit was checked by paulirish@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org Link to the patchset: https://codereview.chromium.org/1905493002/#ps120001 (title: "Fix layout tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905493002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905493002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2016/05/05 at 21:46:26, commit-bot wrote: > Try jobs failed on following builders: > win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) Failed tests don't seem to be related to my change (SVG?). Retrying...
The CQ bit was checked by kdzwinel@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905493002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905493002/120001
On 2016/05/05 at 22:26:28, kdzwinel wrote: > Failed tests don't seem to be related to my change (SVG?). Retrying... Yup seems good. These builders can sometimes be flaky like this.
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Make the extension's sidebar pane auto-resizable. Setting height of the sidebar pane is a relic from the old design where panes were expandable (https://developer.chrome.com/static/images/devtools-panels.png). In the current design, where panes are tabs, it doesn't make sense. BUG=438126 ========== to ========== Make the extension's sidebar pane auto-resizable. Setting height of the sidebar pane is a relic from the old design where panes were expandable (https://developer.chrome.com/static/images/devtools-panels.png). In the current design, where panes are tabs, it doesn't make sense. BUG=438126 Committed: https://crrev.com/7ff544f5fa1d09072def5419d74f8c0e5f42365e Cr-Commit-Position: refs/heads/master@{#391965} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7ff544f5fa1d09072def5419d74f8c0e5f42365e Cr-Commit-Position: refs/heads/master@{#391965}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2155173003/ by caseq@chromium.org. The reason for reverting is: Regression: sources tab extension panels do not render correctly BUG=622456. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
