Closed Bug 1365059 Opened 7 years ago Closed 7 years ago

Update Debugger frontend (5/15/2017)

Categories

(DevTools :: Debugger, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jlast, Assigned: jlast)

References

Details

Attachments

(1 file, 4 obsolete files)

      No description provided.
Attached patch fix_editor_destroy.patch (obsolete) — Splinter Review
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)
See Also: → 1365233
Attached patch fix_editor_destroy.patch (obsolete) — Splinter Review
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)
Comment on attachment 8868119 [details] [diff] [review]
fix_editor_destroy.patch

Review of attachment 8868119 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!
Attachment #8868119 - Flags: review+
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
Attached patch bundle-5-15-1.patch (obsolete) — Splinter Review
Attachment #8868174 - Flags: review?(jdescottes)
Moving the first patch to a dedicated bug, since trees are closed I'll checkin-needed the bug.

Bug 1365314.
Attachment #8868119 - Attachment is obsolete: true
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)
Attached patch bundle-5-15-3.patch (obsolete) — Splinter Review
Attachment #8868174 - Attachment is obsolete: true
Attachment #8868174 - Flags: review?(francesco.lodolo)
Attachment #8868284 - Flags: review?(jdescottes)
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 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+
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.
Attachment #8868284 - Attachment is obsolete: true
Attachment #8868615 - Flags: review?(jdescottes)
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+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78fa048f1254
Update Debugger frontend (5/15/2017). r=jdescottes
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Product: Firefox → DevTools
Assignee: nobody → jlaster
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: