Closed
Bug 1365059
Opened 7 years ago
Closed 7 years ago
Update Debugger frontend (5/15/2017)
Categories
(DevTools :: Debugger, defect)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jlast, Assigned: jlast)
References
Details
Attachments
(1 file, 4 obsolete files)
313.36 KB,
patch
|
jdescottes
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•7 years ago
|
||
We should include this fix together with the next bundle drop. It would be nice to have a mochitest that actually checks the behavior when switching tools while on a breakpoint. I'll see if I have time to do one. In the meantime, try run for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=01db52bc6cff439017d04e43e8b55afb63565432
Attachment #8868040 -
Flags: review?(jlaster)
Comment 2•7 years ago
|
||
Should fix the try failures from previous patch try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ecbe1d74f17a653a9c7973ce73de185ced4cd74
Attachment #8868040 -
Attachment is obsolete: true
Attachment #8868040 -
Flags: review?(jlaster)
Assignee | ||
Comment 3•7 years ago
|
||
Comment on attachment 8868119 [details] [diff] [review] fix_editor_destroy.patch Review of attachment 8868119 [details] [diff] [review]: ----------------------------------------------------------------- Looks great!
Attachment #8868119 -
Flags: review+
Comment 4•7 years ago
|
||
Thanks for the review! As discussed with Jason, let's first land the fix for the source editor. Adding leave-open. Will land when try is green.
Keywords: leave-open
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8868174 -
Flags: review?(jdescottes)
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8eb1b4b9193cb870de19e1127d5ffe2aeb74c48
Comment 7•7 years ago
|
||
Moving the first patch to a dedicated bug, since trees are closed I'll checkin-needed the bug. Bug 1365314.
Updated•7 years ago
|
Attachment #8868119 -
Attachment is obsolete: true
Comment 8•7 years ago
|
||
Comment on attachment 8868174 [details] [diff] [review] bundle-5-15-1.patch Review of attachment 8868174 [details] [diff] [review]: ----------------------------------------------------------------- I guess you are currently fixing the failures for devtools/client/framework/test/browser_browser_toolbox_debugger.js A few things to fix in the patch, have a look at my comments below. Francesco, any additional feedback on the debugger.properties changes? ::: devtools/client/debugger/new/debugger.css @@ +985,5 @@ > background-color: var(--theme-toolbar-background); > margin: 0px; > padding: 0px; > overflow: auto; > + width: calc(100% - 1px); /* 1px fixes the hidden right border */ I'm not really going to review the whole changes but here we are resetting a width which is defined 5 lines above. ::: devtools/client/debugger/new/test/mochitest/browser.ini @@ -58,5 @@ > [browser_dbg-editor-highlight.js] > [browser_dbg-iframes.js] > [browser_dbg_keyboard_navigation.js] > [browser_dbg_keyboard-shortcuts.js] > -skip-if = os == "linux" # bug 1351952 This test was explicitly disabled on linux by Bug 1351952. Unless something was fixed, we should leave it disabled. ::: devtools/client/locales/en-US/debugger.properties @@ +133,5 @@ > +sources.search.key2=CmdOrCtrl+P > + > +# LOCALIZATION NOTE (sources.search.key2b): A second key shortcut to open the > +# search for searching all the source files the debugger has seen. > +sources.search.key2b=CmdOrCtrl+O Since the suffix is traditionally used to bump the version of already existing strings, might be better to use something like "sources.search.alt.key" @@ +152,5 @@ > # the search for re-searching the same search triggered from a sourceSearch > +sourceSearch.search.again.key2=CmdOrCtrl+G > + > +# LOCALIZATION NOTE (sourceSearch.search.againPrev.key2): Key shortcut to re-open > +# the search for re-searching (prev) the same search triggered from a sourceSearch I don't think the localization note is clear, I can't really tell what this does compared to sourceSearch.search.again.key2. After checking the feature I guess the note could be something like "Key shortcut to highlight the previous occurrence of the last search triggered from a source search" (and for the other one "Key shortcut to highlight the next occurrence[...]") @@ +559,5 @@ > # LOCALIZATION NOTE(symbolSearch.search.variablesPlaceholder): The placeholder > # text displayed when the user searches for variables in a file > symbolSearch.search.variablesPlaceholder=Search variables… > > # LOCALIZATION NOTE(symbolSearch.search.key): The shortcut (cmd+shift+o) for nit: The localization note probably should not mention the default english shortcut. @@ +561,5 @@ > symbolSearch.search.variablesPlaceholder=Search variables… > > # LOCALIZATION NOTE(symbolSearch.search.key): The shortcut (cmd+shift+o) for > # searching for a function or variable > +symbolSearch.search.key=CmdOrCtrl+Shift+O This key needs to be updated to symbolSearch.search.key2.
Attachment #8868174 -
Flags: review?(jdescottes) → review?(francesco.lodolo)
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8868174 -
Attachment is obsolete: true
Attachment #8868174 -
Flags: review?(francesco.lodolo)
Attachment #8868284 -
Flags: review?(jdescottes)
Assignee | ||
Comment 10•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6be855cd37ee663609dd516087bc2ca69b335bef
Comment 11•7 years ago
|
||
Comment on attachment 8868284 [details] [diff] [review] bundle-5-15-3.patch Review of attachment 8868284 [details] [diff] [review]: ----------------------------------------------------------------- LGTM if try is green. Restoring r? to flod
Attachment #8868284 -
Flags: review?(jdescottes)
Attachment #8868284 -
Flags: review?(francesco.lodolo)
Attachment #8868284 -
Flags: review+
Comment 12•7 years ago
|
||
Previous try run looks busted, here is a new one : https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b98128060df3cf6144a82184759336647fd5179
Comment 13•7 years ago
|
||
Comment on attachment 8868284 [details] [diff] [review] bundle-5-15-3.patch Review of attachment 8868284 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks.
Attachment #8868284 -
Flags: review?(francesco.lodolo) → review+
Comment 14•7 years ago
|
||
Thanks for the review Francesco! Jason: It looks like this is still frequently failing devtools/client/framework/test/browser_browser_toolbox_debugger.js You can actually easily reproduce this locally. The test works in standalone but if you run it together with other tests or in a loop (`./mach mochitest devtools/client/framework/test/browser_browser_toolbox_debugger.js --run-until-failure`) it will fail the second time. I can't really investigate much more at the moment but I guess you should be able to make progress now that we can reproduce locally.
Assignee | ||
Comment 15•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=44f1c00e5fc94a72288c2947c0d4e138c1b9846f
Assignee | ||
Comment 16•7 years ago
|
||
Attachment #8868284 -
Attachment is obsolete: true
Attachment #8868615 -
Flags: review?(jdescottes)
Comment 17•7 years ago
|
||
Comment on attachment 8868615 [details] [diff] [review] bundle-5-15-4.patch Review of attachment 8868615 [details] [diff] [review]: ----------------------------------------------------------------- LGTM if try is green! (looks ok so far :) )
Attachment #8868615 -
Flags: review?(jdescottes) → review+
Comment 18•7 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/78fa048f1254 Update Debugger frontend (5/15/2017). r=jdescottes
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/78fa048f1254
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 20•6 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
Assignee: nobody → jlaster
You need to log in
before you can comment on or make changes to this bug.
Description
•