769955 - Icons in the app window no longer show up after launching an app, even if the app has an icon

Status: VERIFIED FIXED

(Firefox Graveyard :: Web Apps, defect)

Description

[Jason Smith [:jsmith]]

Attachment: 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]]

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 for replies!)]

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]]

(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 4

[Jason Smith [:jsmith]]

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=6%2F29%2F2012&enddate=6%2F30%2F2012

Comment 5

[Jason Smith [:jsmith]]

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]]

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]]

(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]]

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]]

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 10

[Jason Smith [:jsmith]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?startdate=6%2F28%2F2012&enddate=6%2F29%2F2012

Comment 11

[:Felipe Gomes (needinfo for replies!)]

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]]

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]]

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]]

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]]

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]]

(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 for replies!)]

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 for replies!)]

Attachment: 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]]

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 for replies!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf1b8a5acaf

Comment 21

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/fdf1b8a5acaf