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
:
: J. Ryan Stinnett [:jryans] (use ni?)
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 User image Paul Rouget [:paul] 2012-05-22 09:02:21 PDT
We should restore the latest size and preset.
Comment 1 User image Paul Rouget [:paul] 2012-05-22 09:03:00 PDT
Created attachment 626043 [details] [diff] [review]
patch v1
Comment 2 User image Paul Rouget [:paul] 2012-05-28 11:24:12 PDT
Created attachment 627753 [details] [diff] [review]
patch v1

rebased
Comment 3 User image 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 User image 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 User image Paul Rouget [:paul] 2012-06-02 10:58:39 PDT
Created attachment 629500 [details] [diff] [review]
v1.1

addressed dave's comments
Comment 6 User image 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 User image Paul Rouget [:paul] 2012-06-03 04:37:27 PDT
https://hg.mozilla.org/integration/fx-team/rev/ec971b2d6a30
Comment 8 User image 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.