Closed Bug 1262443 Opened 8 years ago Closed 8 years ago

Migrate inspector.xul to HTML

Categories

(DevTools :: Inspector, enhancement, P1)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox52 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 52
Iteration:
52.1 - Oct 3
Tracking Flags:
Tracking Status
firefox52 --- verified

People

(Reporter: pbro, Assigned: Honza)

References

(Blocks 1 open bug)

Details

(Whiteboard: [devtools-html])

Attachments

(2 files, 2 obsolete files)

bug1262443-tests.patch
9.88 KB, patch
pbro
: review+
Details | Diff | Splinter Review
bug1262443.patch
6.26 KB, patch
Honza
: review+
Details | Diff | Splinter Review 
Once the most important parts of the inspector have been migrated to HTML (bug 1259812, bug 1259819, bug 1260552), we should make inspector.xul be inspector.xhtml instead and migrate the remaining elements to HTML, like the toolbar and search field.

This also depends on contextmenus and panels being converted to HTML equivalent too (bug 1259834, bug 1257613).
Depends on: 1260493
Severity: normal → enhancement
Alias: devtools-html-inspector
No longer blocks: devtools-html-1
Whiteboard: [meta]
Keywords: meta
Brian, shouldn't this be in devtools.html track 2 MVP? I think we might have missed it during breakdown.
Flags: needinfo?(bgrinstead)
(In reply to Patrick Brosset <:pbro> from comment #1)
> Brian, shouldn't this be in devtools.html track 2 MVP? I think we might have
> missed it during breakdown.

In some sense this is the end goal of all of the work between tracks 1 and 2.  I agree we should be tracking it - and I guess track 2 is the logical place to put it.  I think we can give it a try once bug 1259819 and bug 1260552 are fixed.
Flags: needinfo?(bgrinstead)
Making this not a meta bug, this can be a work bug for converting inspector.xul to inspector.xhtml
Blocks: devtools-html-phase2
Keywords: meta
Whiteboard: [meta] → [devtools-html][triage]
Alias: devtools-html-inspector
Whiteboard: [devtools-html][triage] → [devtools-html]
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
Priority: P2 → --
What is the difference between this bug and bug 1271127 ?
Flags: qe-verify?
Priority: -- → P2
(In reply to David Bruant from comment #5)
> What is the difference between this bug and bug 1271127 ?

No difference, thanks for point that out.
Flags: qe-verify? → qe-verify+
QA Contact: petruta.rasa
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1
After discussing with Honza, the inspector.xul is being migrated to XHTML as part of Bug 1260552. We will just check if anything remains to be done after it lands.
Iteration: 51.1 - Aug 15 → 51.2 - Aug 29
Status: ASSIGNED → NEW
Iteration: 51.2 - Aug 29 → ---
Priority: P1 → P2
Attached patch bug1262443.patch (obsolete) — — Splinter Review 
I was experimenting with this a bit and here is preliminary patch.
It's based on patches from bug 1260552 and a patch from bug 1294186.

Honza
Assignee: jdescottes → odvarko
Status: NEW → ASSIGNED
Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59c75ae3c64d

Test failures are mostly related to CSS property editing (Rule View) and clipboard (cut/copy/paste context menu items). A lot of failures might be related to wrong focus management.

Honza
Iteration: --- → 51.3 - Sep 19
Priority: P2 → P1
Attached patch bug1262443.patch (obsolete) — — Splinter Review 
Patch updated.

It depends on patch from bug 1294186

Honza
Attachment #8790660 - Attachment is obsolete: true
Attachment #8791891 - Flags: review?(pbrosset)
Fixing tests.

Honza
Attachment #8791894 - Flags: review?(pbrosset)
Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f57981f2a52

There are some oranges, but I believe that they are unrelated.

Honza
Comment on attachment 8791891 [details] [diff] [review]
bug1262443.patch

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

It looks like you deleted inspector.xul and created inspector.xhtml. Could you use 'move' instead? So that history is preserved?
Looks good otherwise.

::: devtools/client/inspector/inspector.xhtml
@@ +28,5 @@
> +          src="chrome://devtools/content/shared/theme-switching.js"></script>
> +</head>
> +
> +<body class="theme-body devtools-monospace" role="application">
> +  <html:div class="inspector-responsive-container theme-body inspector">

Can we not get rid of the all the html namespaces throughout the file now?
Attachment #8791891 - Flags: review?(pbrosset) → review+
Comment on attachment 8791894 [details] [diff] [review]
bug1262443-tests.patch

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

Simple test changes with comments, works for me!
Attachment #8791894 - Flags: review?(pbrosset) → review+
Attached patch bug1262443.patch — — Splinter Review 
(In reply to Patrick Brosset <:pbro> from comment #15)
> Comment on attachment 8791891 [details] [diff] [review]
> bug1262443.patch
> 
> Review of attachment 8791891 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks like you deleted inspector.xul and created inspector.xhtml. Could
> you use 'move' instead? So that history is preserved?
Done

> Looks good otherwise.
> 
> ::: devtools/client/inspector/inspector.xhtml
> @@ +28,5 @@
> > +          src="chrome://devtools/content/shared/theme-switching.js"></script>
> > +</head>
> > +
> > +<body class="theme-body devtools-monospace" role="application">
> > +  <html:div class="inspector-responsive-container theme-body inspector">
> 
> Can we not get rid of the all the html namespaces throughout the file now?
Yes, I'll do it in separate bug (bug 1304061) just to keep this patch as simple as possible.

I am going to include both patches from this bug into bug 1260552 so, we don't mix XUL & XHTML. It makes tests more stable (patch from bug 1260552 was backed out).

Also, the patch here doesn't depend on bug 1294186.

Let's keep this bug open for verification.

Thanks for the review Patrick!

Honza
Attachment #8791891 - Attachment is obsolete: true
Attachment #8792918 - Flags: review+
Iteration: 51.3 - Sep 19 → 52.1 - Oct 3
https://hg.mozilla.org/mozilla-central/rev/da144bd5bc51
https://hg.mozilla.org/mozilla-central/rev/36373fbb7811
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Depends on: 1304727
Depends on: 1304732
Depends on: 1305007
Depends on: 1305056
No longer depends on: 1305056
Depends on: 1305056
Depends on: 1305503
I ran an overall check-up, no issues regressed.Also I verified all it's dependencies. I will mark this bug as verified.

Cheers!
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Flags: qe-verify+
See Also: → 1316296
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: