Comment 1 Brian Nicholson (:bnicholson) 2012-06-27 16:55:26 PDT

Created attachment 637307 [details] [diff] [review]
patch

Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-30 06:46:36 PDT

Comment on attachment 637307 [details] [diff] [review]
patch

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>     this.id = ++gTabIDFactory;
>+    this.desktopMode = aParams.desktopMode == true;

Use parens to make this a bit more readable:

      this.desktopMode = (aParams.desktopMode == true);

but I think we need more since desktopMode is optional and we want to be more explicit:

      this.desktopMode = ("desktopMode" in aParams) ? aParams.desktopMode : false;

r+ with the nits

Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-30 06:49:16 PDT

Comment on attachment 637307 [details] [diff] [review]
patch

>diff --git a/mobile/android/components/SessionStore.js b/mobile/android/components/SessionStore.js

>-          let params = { selected: isSelected, delayLoad: true, title: entry.title };
>+          let params = {
>+            selected: isSelected,
>+            delayLoad: true,
>+            title: entry.title,
>+            desktopMode: tabData.desktopMode == true
>+          };

Actually, what if the sessionstore.js file has no "desktopMode" data? Will this throw an exception?

I guess it won't and desktopMode would be null which evaluates to false. So we are OK.

Comment 4 Brian Nicholson (:bnicholson) 2012-07-02 12:42:51 PDT

http://hg.mozilla.org/integration/mozilla-inbound/rev/4a5f794edd2d

Comment 5 Ryan VanderMeulen [:RyanVM] 2012-07-02 19:01:28 PDT

https://hg.mozilla.org/mozilla-central/rev/4a5f794edd2d

Comment 6 Brian Nicholson (:bnicholson) 2012-07-11 23:21:42 PDT

Uplifted as part of bug 766406.