Last Comment Bug 757477 - [Responsive Mode] restore previous size / preset
: [Responsive Mode] restore previous size / preset
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Firefox 15
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-22 09:02 PDT by Paul Rouget [:paul]
Modified: 2012-06-03 13:46 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (11.06 KB, patch)
2012-05-22 09:03 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1 (11.09 KB, patch)
2012-05-28 11:24 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
v1.1 (15.46 KB, patch)
2012-06-02 10:58 PDT, Paul Rouget [:paul]
dcamp: review+
Details | Diff | Splinter Review
un-bitrotted (14.91 KB, patch)
2012-06-02 16:42 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2012-05-22 09:02:21 PDT
We should restore the latest size and preset.
Comment 1 Paul Rouget [:paul] 2012-05-22 09:03:00 PDT
Created attachment 626043 [details] [diff] [review]
patch v1
Comment 2 Paul Rouget [:paul] 2012-05-28 11:24:12 PDT
Created attachment 627753 [details] [diff] [review]
patch v1

rebased
Comment 3 Dave Camp (:dcamp) 2012-05-29 08:32:24 PDT
Comment on attachment 627753 [details] [diff] [review]
patch v1

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

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +76,5 @@
>      this.presets = [{custom: true}];
>    }
>  
>    // Default size. The first preset (custom) is the one that will be used.
>    let bbox = this.stack.getBoundingClientRect();

Do you need this bbox

@@ +81,5 @@
> +
> +  try {
> +    this.presets[0].width = Services.prefs.getIntPref("devtools.responsiveUI.customWidth");
> +    this.presets[0].height = Services.prefs.getIntPref("devtools.responsiveUI.customHeight");
> +    this.currentPreset = Services.prefs.getIntPref("devtools.responsiveUI.currentPreset");

Should there be a sanity check on these values?  Can a really-large custom size cause any real problems?

Should the preset be some sort of absolute string rather than an integer index?  That could make later updates to the preset list less fragile.

@@ +306,5 @@
>     */
>    rotate: function RUI_rotate() {
>      this.setSize(this.currentHeight, this.currentWidth);
> +    if (this.currentPreset == 0)
> +      this.saveCustomSize();

Do you save the rotation value anywhere?
Comment 4 Dave Camp (:dcamp) 2012-05-29 08:40:04 PDT
(In reply to Dave Camp (:dcamp) from comment #3)
> Comment on attachment 627753 [details] [diff] [review]
> patch v1
> 
> Review of attachment 627753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/responsivedesign/responsivedesign.jsm
> @@ +76,5 @@
> >      this.presets = [{custom: true}];
> >    }
> >  
> >    // Default size. The first preset (custom) is the one that will be used.
> >    let bbox = this.stack.getBoundingClientRect();
> 
> Do you need this bbox
... outside of the catch block?
Comment 5 Paul Rouget [:paul] 2012-06-02 10:58:39 PDT
Created attachment 629500 [details] [diff] [review]
v1.1

addressed dave's comments
Comment 6 Dave Camp (:dcamp) 2012-06-02 16:42:44 PDT
Created attachment 629533 [details] [diff] [review]
un-bitrotted

I had bitrotted you pretty badly, so I fixed it.
Comment 7 Paul Rouget [:paul] 2012-06-03 04:37:27 PDT
https://hg.mozilla.org/integration/fx-team/rev/ec971b2d6a30
Comment 8 Rob Campbell [:rc] (:robcee) 2012-06-03 13:46:21 PDT
https://hg.mozilla.org/mozilla-central/rev/ec971b2d6a30

Note You need to log in before you can comment on or make changes to this bug.