Closed Bug 882353 Opened 6 years ago Closed 6 years ago

Reduce duplication of updateWidgetStyle in CustomizableWidgets.jsm

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jaws, Assigned: darshanapadmadas)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M9][Australis:P5][good first bug][mentor=Gijs][lang=js])

Attachments

(1 file, 1 obsolete file)

The function for updateWidgetStyle in CustomizableWidgets.jsm is duplicated for the zoom-controls and edit-controls.

It varies only slightly in the zoom-controls case by setting the reset button hidden if it's in the panel and also setting the noautoclose attribute. We should be able to remove the duplication but keep the same behavior.
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P5]
To fix this bug, the "updateWidgetStyle" function should be moved outside of the two widgets that currently have one, possibly be renamed to something like "updateCombinedWidgetStyle" and take care of both the zoom controls and edit controls correctly. Here are the original functions:

http://mxr.mozilla.org/projects-central/source/ux/browser/components/customizableui/src/CustomizableWidgets.jsm?rev=9737cc4b6837#339

http://mxr.mozilla.org/projects-central/source/ux/browser/components/customizableui/src/CustomizableWidgets.jsm?rev=9737cc4b6837#461
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M?][Australis:P5][good first bug][mentor=Gijs][lang=js]
Hello!
I'd like to get started on this bug.Could you assign this to me?
(In reply to Darshana Padmadas from comment #2)
> Hello!
> I'd like to get started on this bug.Could you assign this to me?

Done! Let me know if you have any questions. :-)
Assignee: nobody → darshanapadmadas
Status: NEW → ASSIGNED
Attached patch output.txt (obsolete) — Splinter Review
Attachment #826763 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 826763 [details] [diff] [review]
output.txt

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

This is close, but to be honest I have no idea how it worked - the attrs.noautoclose setting can't be doing the right thing if you're setting it before declaring attrs. Please apply the following corrections and submit a new version of the patch.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +38,4 @@
>    }
>  }
>  
> +

Nit: there's already spacing here, no need to add extra lines.

@@ +39,5 @@
>  }
>  
> +
> +
> +function updateCombinedWidgetStyle(aArea,modifyAutoclose) {

Nit: space before "modifyAutoclose"

Nit: please use "aModifyAutoclose" as the name for this parameter

@@ +40,5 @@
>  
> +
> +
> +function updateCombinedWidgetStyle(aArea,modifyAutoclose) {
> +        let inPanel = (aArea == CustomizableUI.AREA_PANEL);

Nit: Please use two spaces as indentation. Don't use tabs, don't use 8 spaces or whatever this is.

@@ +41,5 @@
> +
> +
> +function updateCombinedWidgetStyle(aArea,modifyAutoclose) {
> +        let inPanel = (aArea == CustomizableUI.AREA_PANEL);
> +	if (modifyAutoclose){

Nit: space before {

@@ +42,5 @@
> +
> +function updateCombinedWidgetStyle(aArea,modifyAutoclose) {
> +        let inPanel = (aArea == CustomizableUI.AREA_PANEL);
> +	if (modifyAutoclose){
> +	  attrs.noautoclose = inPanel? true:null;

Nit: space before "?" and around ":"

@@ +47,5 @@
> +	}
> +        let cls = inPanel ? "panel-combined-button" : "toolbarbutton-1";
> +        if (!aArea)
> +          cls = null;
> +        let attrs = {class: cls};

This won't work. You should declare and set attrs before setting the noautoclose property on it. Move the if (modifyAutoclose) block down to below this line.

@@ +365,4 @@
>          updateZoomResetButton();
>        }
>  
> +      

Nit: Please remove this empty line.

@@ +373,5 @@
>  
> +          updateCombinedWidgetStyle(aArea,true);
> +	  updateZoomResetButton();
> +	  
> +	  

Nit: Please remove these two extra empty lines

@@ +392,4 @@
>  
>            // When a widget is demoted to the palette ('removed'), it's visual
>            // style should change.
> +          updateCombinedWidgetStyle();

If you do this, the attribute won't get removed. You should pass (null, true) as arguments.

@@ +399,5 @@
>          onWidgetReset: function(aWidgetId) {
>            if (aWidgetId != this.id)
>              return;
> +          updateCombinedWidgetStyle(this.currentArea,true);
> +	   updateZoomResetButton();

Nit: please align this with the line above.

@@ +406,5 @@
>          onWidgetMoved: function(aWidgetId, aArea) {
>            if (aWidgetId != this.id)
>              return;
> +          updateCombinedWidgetStyle(aArea,true);
> +	   updateZoomResetButton();

Nit: please align this with the line above.
Attachment #826763 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch new_output.txtSplinter Review
Attachment #826763 - Attachment is obsolete: true
Attachment #827212 - Flags: review?(gijskruitbosch+bugs)
https://hg.mozilla.org/projects/ux/rev/8e9d485dd741
Whiteboard: [Australis:M?][Australis:P5][good first bug][mentor=Gijs][lang=js] → [Australis:M9][Australis:P5][good first bug][mentor=Gijs][lang=js][fixed-in-ux]
Comment on attachment 827212 [details] [diff] [review]
new_output.txt

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

Great! There were still a bunch of whitespace changes that needed correcting, but I've gone ahead and done that for you and landed the patch. Thank you! :-)
Attachment #827212 - Flags: review?(gijskruitbosch+bugs) → review+
Keywords: checkin-needed
This doesn't need to be checked in, I already did so. See comment 7 and comment 8.
Keywords: checkin-needed
Alright.Thanks for all your help. :)
So both of us completely missed that you kind of need to pass node to this function, because in that scope node doesn't exist. Followup fix landed:

https://hg.mozilla.org/projects/ux/rev/5f73d1f59d3b
Oh I'm so sorry I missed that!Has this issue been fixed now? Any more changes to be made?
https://hg.mozilla.org/mozilla-central/rev/8e9d485dd741
https://hg.mozilla.org/mozilla-central/rev/5f73d1f59d3b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P5][good first bug][mentor=Gijs][lang=js][fixed-in-ux] → [Australis:M9][Australis:P5][good first bug][mentor=Gijs][lang=js]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.