Closed Bug 1370324 Opened 3 years ago Closed 3 years ago

Update Debugger frontend (6/5/2017)

Categories

(DevTools :: Debugger, defect)

defect
Not set

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: jlast, Assigned: jlast)

References

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Attached patch 6-5.patch (obsolete) — Splinter Review
Attachment #8874544 - Flags: review?(jdescottes)
Attached patch 6-5-1.patch (obsolete) — Splinter Review
Attachment #8874544 - Attachment is obsolete: true
Attachment #8874544 - Flags: review?(jdescottes)
Attachment #8874653 - Flags: review?(jdescottes)
Attached patch 6-5-2.patch (obsolete) — Splinter Review
Attachment #8874653 - Attachment is obsolete: true
Attachment #8874653 - Flags: review?(jdescottes)
Attachment #8874656 - Flags: review?(jdescottes)
Comment on attachment 8874656 [details] [diff] [review]
6-5-2.patch

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

- the commit message doesn't mention any sha1/tag
- patch needs rebasing
- few nits

Some additional questions: 
 - the previous try run had failures for browser_dbg-breaking.js, I suppose the last patches attached are addressing this?
 - the patch is unusually large here (3.6MB), is it normal?

Thanks!

::: devtools/client/debugger/new/test/mochitest/.eslintrc
@@ +25,5 @@
>      "requestLongerTimeout": false,
>      "SimpleTest": false,
>      "SpecialPowers": false,
>      "TestUtils": false,
> +    "thisTestLeaksUncaughtRejectionsAndShouldBeFixed": false,

Your patch doesn't apply cleanly because of this file, please rebase.

::: devtools/client/debugger/new/test/mochitest/browser.ini
@@ +50,5 @@
>  [browser_dbg-expressions.js]
>  [browser_dbg-scopes.js]
>  [browser_dbg-chrome-create.js]
>  [browser_dbg-chrome-debugging.js]
> +skip-if = true

Is there an issue/bug tracking this?

::: devtools/client/debugger/new/test/mochitest/head.js
@@ +31,5 @@
>   * @module mochitest/helpers
>   * @parent mochitest
>   */
>  
> +// shared-head.js handles

? Should be restored I guess

::: devtools/client/themes/toolbox.css
@@ +171,5 @@
>    background-color: var(--theme-toolbar-hover-active);
>  }
>  
> +.devtools-tab:not(.selected).highlighted {
> +  background-color: var(--theme-toolbar-background-alt)

missing semi-colon

::: devtools/client/themes/variables.css
@@ +149,5 @@
>    --theme-sidebar-background: #fcfcfc;
>    --theme-contrast-background: #e6b064;
>  
>    --theme-tab-toolbar-background: rgb(240, 240, 240) linear-gradient(rgba(255, 255, 255, 0.8), transparent);
> +  --theme-toolbar-background-hover: rgba(221, 225, 228, 0.66);

FYI, firebug theme inherits light theme, so if the value is exactly the same you don't have to define it.
Attachment #8874656 - Flags: review?(jdescottes)
Attached patch 6-5-3.patch (obsolete) — Splinter Review
Attachment #8874656 - Attachment is obsolete: true
Attachment #8874827 - Flags: review?(jdescottes)
Comment on attachment 8874827 [details] [diff] [review]
6-5-3.patch

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

Bundle looks good, as discussed we spotted two bugs:
- breakpoints are sliding when reloading the content page
- call stack is not showing all the frames when disabling framework grouping

Waiting for a bundle addressing the first issue at least.
Attachment #8874827 - Flags: review?(jdescottes)
Attached patch 6-5-4.patchSplinter Review
Attachment #8874827 - Attachment is obsolete: true
Attachment #8874949 - Flags: review?(jdescottes)
Thanks julian, I've fixed the first issue. We can punt on the call stack. Few people use it and I don't think it will be noticed before the next release goes out.
I forgot to mention, but the newest patch is much smaller. Ryan found the regression and fixed it here: https://github.com/devtools-html/debugger.html/pull/3104
Comment on attachment 8874949 [details] [diff] [review]
6-5-4.patch

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

Looks good to me if try is green, thanks!

::: devtools/client/debugger/new/debugger.css
@@ +1646,5 @@
>    cursor: pointer;
>  }
>  
>  .selected-token {
> +  background-color: var(--theme-highlight-yellow);

--theme-search-overlays-semitransparent is no longer used anywhere and should be removed from variables.css

@@ +1748,5 @@
> +.theme-light {
> +  --theme-conditional-breakpoint-color: #ccd1d5;
> +}
> +
> +.theme-firebug {

not only could this be rewritten as .theme-light, .theme firebug {}, but as mentioned earlier theme firebug inherits light theme.
Attachment #8874949 - Flags: review?(jdescottes) → review+
(In reply to Jason Laster [:jlast] from comment #11)
> I forgot to mention, but the newest patch is much smaller. Ryan found the
> regression and fixed it here:
> https://github.com/devtools-html/debugger.html/pull/3104

Great job!
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f58a80807c73
Update Debugger frontend (v0.5) (6/5/2017). r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/f58a80807c73
Assignee: nobody → jlaster
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.