1049831 - Remove app_chrome.js dead code

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: dead.app_chrome.code.patch

Hey Kevin,

there is a lot of dead code in app_chrome.js. Let's remove it.

At first I didn't really want to do that now but I realized that it ask Chris to do some additional code, and I have to review some code that is pure dead code...

If you want to try out this patch: https://github.com/vingtetun/gaia/tree/app_chrome-cleanup

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: dead.app_chrome.code.patch

Oups. I forgot to |git add -u| my last change!

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8468714 [details] [diff] [review]
dead.app_chrome.code.patch

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

Seems fine to me. Thanks!

::: apps/system/style/wrapper/wrapper.css
@@ +27,3 @@
>    pointer-events: auto;
> +  display: none;
> +} 

Extra space?

Comment 4

[Chris Lord [:cwiiis]]

Comment on attachment 8468714 [details] [diff] [review]
dead.app_chrome.code.patch

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

::: apps/system/test/unit/app_chrome_test.js
@@ -289,5 @@
> -      var app = new AppWindow(fakeAppConfig1);
> -      var chrome = new AppChrome(app);
> -      var stubAddBookmark = this.sinon.stub(chrome, 'onAddBookmark');
> -      chrome.handleEvent({ type: 'click', target: chrome.bookmarkButton });
> -      assert.isTrue(stubAddBookmark.called);

This test could be converted to use the overflow menu's add-to-home button instead of being removed? Just call showOverflowMenu, replace onAddBookmark with onAddToHome and bookmarkButton with addToHomeButton.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Chris Lord [:cwiiis] from comment #4)
> Comment on attachment 8468714 [details] [diff] [review]
> dead.app_chrome.code.patch
> 
> Review of attachment 8468714 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: apps/system/test/unit/app_chrome_test.js
> @@ -289,5 @@
> > -      var app = new AppWindow(fakeAppConfig1);
> > -      var chrome = new AppChrome(app);
> > -      var stubAddBookmark = this.sinon.stub(chrome, 'onAddBookmark');
> > -      chrome.handleEvent({ type: 'click', target: chrome.bookmarkButton });
> > -      assert.isTrue(stubAddBookmark.called);
> 
> This test could be converted to use the overflow menu's add-to-home button
> instead of being removed? Just call showOverflowMenu, replace onAddBookmark
> with onAddToHome and bookmarkButton with addToHomeButton.

Fixed.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/d2d93b782446fe5d1b4bba9305ce4176669912be