Closed Bug 1269497 Opened 5 years ago Closed 5 years ago

Add 'visible' support to MenuItem API

Categories

(DevTools :: Framework, enhancement, P1)

46 Branch
enhancement

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Iteration:
49.1 - May 9
Tracking Status
firefox49 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

(Whiteboard: [devtools-html])

Attachments

(3 files)

This option allows a menuitem to not be shown: https://github.com/electron/electron/blob/master/docs/api/menu-item.md#class-menuitem
Flags: qe-verify-
Comment on attachment 8747963 [details]
MozReview Request: Bug 1269497 - Add 'visible' support to MenuItem API;r=jdescottes

https://reviewboard.mozilla.org/r/50085/#review46941

Looks good to me!

::: devtools/client/framework/menu-item.js:47
(Diff revision 1)
>      disabled = false,
>      label = "",
>      id = null,
>      submenu = null,
>      type = "normal",
> +    visible = true,

Please update the method's JS doc which refers to "visible" as a missing feature.
Attachment #8747963 - Flags: review?(jdescottes) → review+
Comment on attachment 8747964 [details]
MozReview Request: Bug 1269497 - Provide toolbox.win getter to make lines shorter in tests;r=jdescottes

https://reviewboard.mozilla.org/r/50087/#review46937

No longer will we have to type toolbox.doc.defaultView, thanks :)

::: devtools/client/framework/toolbox.js:283
(Diff revision 1)
>  
>    /**
> +   * Shortcut to the window containing the toolbox UI
> +   */
> +  get win() {
> +    return this.doc.defaultView;

(not "exactly" an issue, just want to bring this to your attention)

Here I think this.doc.defaultView === this.frame.contentWindow, so we could clean up references to :
- this.frame.contentWindow in toolbox.js 
- toolbox.frame.contentWindow in other files

nit : The getter could point to this.frame.contentWindow

::: devtools/client/inspector/inspector-panel.js:992
(Diff revision 1)
>      this._markupFrame.contentWindow.focus();
>  
>      this._markupBox.removeAttribute("collapsed");
>  
> -    let controllerWindow = this._toolbox.doc.defaultView;
> +    let controllerWindow = this._toolbox.win;
>      this.markup = new MarkupView(this, this._markupFrame, controllerWindow);

"this._toolbox.win" is just one char longer than controllerWindow, could get rid of the variable (or rename it, "controller" feels out of place in this file).
Attachment #8747964 - Flags: review?(jdescottes) → review+
Comment on attachment 8747965 [details]
MozReview Request: Bug 1269497 - eslint fixes for Menu API;r=jdescottes

https://reviewboard.mozilla.org/r/50089/#review46929

Thanks!
Attachment #8747965 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes] from comment #5)
> Comment on attachment 8747964 [details]
> ::: devtools/client/framework/toolbox.js:283
> (Diff revision 1)
> >  
> >    /**
> > +   * Shortcut to the window containing the toolbox UI
> > +   */
> > +  get win() {
> > +    return this.doc.defaultView;
> 
> (not "exactly" an issue, just want to bring this to your attention)
> 
> Here I think this.doc.defaultView === this.frame.contentWindow, so we could
> clean up references to :
> - this.frame.contentWindow in toolbox.js 
> - toolbox.frame.contentWindow in other files
> 
> nit : The getter could point to this.frame.contentWindow
> 

I'll file a follow up bug to convert instances of toolbox.frame.contentWindow to use toolbox.win

> ::: devtools/client/inspector/inspector-panel.js:992
> (Diff revision 1)
> >      this._markupFrame.contentWindow.focus();
> >  
> >      this._markupBox.removeAttribute("collapsed");
> >  
> > -    let controllerWindow = this._toolbox.doc.defaultView;
> > +    let controllerWindow = this._toolbox.win;
> >      this.markup = new MarkupView(this, this._markupFrame, controllerWindow);
> 
> "this._toolbox.win" is just one char longer than controllerWindow, could get
> rid of the variable (or rename it, "controller" feels out of place in this
> file).

Done, thanks
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Blocks: 1269763
Iteration: --- → 49.1 - May 9
Priority: -- → P1
Whiteboard: [devtools-html][triage] → [devtools-html]
https://hg.mozilla.org/mozilla-central/rev/ef342caf249a
https://hg.mozilla.org/mozilla-central/rev/fec2918b252a
https://hg.mozilla.org/mozilla-central/rev/6b8ff7012009
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.