Last Comment Bug 769955 - Icons in the app window no longer show up after launching an app, even if the app has an icon
: Icons in the app window no longer show up after launching an app, even if the...
Status: VERIFIED FIXED
[qa!]
: regression
Product: Firefox Graveyard
Classification: Graveyard
Component: Web Apps (show other bugs)
: unspecified
: All Windows 7
: -- normal
: Firefox 16
Assigned To: :Felipe Gomes (needinfo me!)
: Jason Smith [:jsmith]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-30 14:14 PDT by Jason Smith [:jsmith]
Modified: 2016-02-04 15:00 PST (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Line Rage No Icon (4.52 KB, image/png)
2012-06-30 14:14 PDT, Jason Smith [:jsmith]
no flags Details
Patch (852 bytes, patch)
2012-07-03 16:14 PDT, :Felipe Gomes (needinfo me!)
myk: review+
Details | Diff | Splinter Review

Description Jason Smith [:jsmith] 2012-06-30 14:14:09 PDT
Created attachment 638145 [details]
Line Rage No Icon

Steps:

1. Install an app with an icon
2. Launch the app

Expected:

The app's icon should be shown in the top left portion of the window.

Actual:

The executable icon is shown instead. This is a regression - this definitely wasn't happening before. See screenshot.

Tested on 6/30 Mozilla inbound build.
Comment 1 Jason Smith [:jsmith] 2012-06-30 16:25:49 PDT
Just checked the past nightly - this is working 6/30 nightly, busted in 6/30 inbound. Got a strong hunch the patch in bug 759619 caused this regression. 

Felipe - Any ideas?
Comment 2 :Felipe Gomes (needinfo me!) 2012-07-02 03:36:25 PDT
The patch in bug 759619 would be very unlikely to cause this bug. Most likely it was some platform change on the same day. is it still happening? Let's try to find a better regression window. does inbound have hourly builds?  after we have a narrower window I can bisect to find out what caused it
Comment 3 Jason Smith [:jsmith] 2012-07-02 08:33:33 PDT
(In reply to Felipe Gomes (:felipe) from comment #2)
> The patch in bug 759619 would be very unlikely to cause this bug. Most
> likely it was some platform change on the same day. is it still happening?
> Let's try to find a better regression window. does inbound have hourly
> builds?  after we have a narrower window I can bisect to find out what
> caused it

Yes it's still happening on the 7/2 nightly build. Let me look into this.
Comment 5 Jason Smith [:jsmith] 2012-07-02 18:22:42 PDT
Not Working:

- ftp://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-win32/1341137137/
**** Changeset - d9d61d199b11

Working: 

- ftp://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-win32/1341094431/
**** Changeset - d9d61d199b11
Comment 6 Jason Smith [:jsmith] 2012-07-02 18:23:50 PDT
http://hg.mozilla.org/mozilla-central/rev/d9d61d199b11 <-- does have a change to a webapps file, could something have not gone right here? 

Tim - Could your patch be causing this regression?
Comment 7 Jason Smith [:jsmith] 2012-07-02 18:31:18 PDT
(In reply to Jason Smith [:jsmith] from comment #6)
> http://hg.mozilla.org/mozilla-central/rev/d9d61d199b11 <-- does have a
> change to a webapps file, could something have not gone right here? 
> 
> Tim - Could your patch be causing this regression?

Change that, it isn't that patch. let me recheck that regression range in comment 5 too.
Comment 8 Jason Smith [:jsmith] 2012-07-02 18:47:32 PDT
Last Working Build:

http://hg.mozilla.org/mozilla-central/rev/f08d285b63b0

First build Failed:

http://hg.mozilla.org/mozilla-central/rev/4c2ddc60f360
Comment 9 Jason Smith [:jsmith] 2012-07-02 18:50:17 PDT
Ugh. This changeset in this build that failed is huge.

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f08d285b63b0&tochange=4c2ddc60f360

Gavin or Felipe - Is there any other way we could narrow this regression range down?
Comment 11 :Felipe Gomes (needinfo me!) 2012-07-02 20:41:11 PDT
i'm trying to bisect that range but I think I did something wrong with my commands so i'm not sure if it's leading somewhere or if I'll need to start over
Comment 12 Jason Smith [:jsmith] 2012-07-02 20:54:16 PDT
Getting closer. I know the build with this changeset below does not work.

http://hg.mozilla.org/integration/mozilla-inbound/rev/df25da024956
Comment 13 Jason Smith [:jsmith] 2012-07-02 21:12:04 PDT
Digging into this more, it looks like this was broken at the time that your patch landed as well:

https://hg.mozilla.org/integration/mozilla-inbound/rev/72a0595d29a1
Comment 14 Jason Smith [:jsmith] 2012-07-02 21:14:53 PDT
Got a regression range:

Last Working:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b5f8c30088c

First Failed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/72a0595d29a1

Which means that is actually is bug 759619 that caused this.
Comment 15 Jason Smith [:jsmith] 2012-07-02 21:21:31 PDT
For my own sanity to confirm comment 14 is right or not - What happens if you backout the patch for bug 759619?
Comment 16 Jason Smith [:jsmith] 2012-07-02 21:22:08 PDT
(In reply to Jason Smith [:jsmith] from comment #15)
> For my own sanity to confirm comment 14 is right or not - What happens if
> you backout the patch for bug 759619?

(Note: I don't mean literally back it out from mozilla central, just mean to say - backout and test it)
Comment 17 :Felipe Gomes (needinfo me!) 2012-07-02 21:54:24 PDT
Thanks, I backed out and tested it and confirmed that it caused this bug. Sorry for leading in the wrong direction, I have no idea how it can have caused that. I'll investigate soon.
Comment 18 :Felipe Gomes (needinfo me!) 2012-07-03 16:14:49 PDT
Created attachment 638904 [details] [diff] [review]
Patch

The problem is, in nsXULWindow.cpp [1], the code uses the window id as the name of the icon file to use (+ .ico), or default.ico if the window has no id. Since we added id-"webapprt-window" in bug 759619, that no longer works.

We could change the icon name to webapprt-window, or change the window name to default. Since the icon is created during installation time, I don't want to leave existing installations permanently broken (as their icon have been created as default.ico), so this patch changes the window id.

http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsXULWindow.cpp#1361
Comment 19 Myk Melez [:myk] [@mykmelez] 2012-07-05 13:33:32 PDT
Comment on attachment 638904 [details] [diff] [review]
Patch

(In reply to Felipe Gomes (:felipe) from comment #18)
> The problem is, in nsXULWindow.cpp [1], the code uses the window id as the
> name of the icon file to use (+ .ico), or default.ico if the window has no
> id. Since we added id-"webapprt-window" in bug 759619, that no longer works.
> 
> We could change the icon name to webapprt-window, or change the window name
> to default. Since the icon is created during installation time, I don't want
> to leave existing installations permanently broken (as their icon have been
> created as default.ico), so this patch changes the window id.

It's ok to break apps installed by Nightly/Aurora users.  We should not do so arbitrarily, but we need to be able to do so in order to make useful changes.  And occasional bustage is part of what you sign up for when you install and use those builds.

In fact this change presumably also results in bustage, as it causes the runtime to ignore the previous window position/size.  Nevertheless, the change is ok, although it's unfortunate that the icon name is determined in this matter.  I would prefer a more explicit mapping.
Comment 20 :Felipe Gomes (needinfo me!) 2012-07-05 15:09:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf1b8a5acaf

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