Closed Bug 992139 Opened 6 years ago Closed 6 years ago

[mozversion] Set "application_display_name" to "application_name" if CodeName doesn't exist

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(firefox31 fixed)

RESOLVED FIXED
mozilla31
Tracking Status
firefox31 --- fixed

People

(Reporter: cosmin-malutan, Assigned: cosmin-malutan)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [mentor=whimboo][lang=py][good first bug])

Attachments

(1 file, 4 obsolete files)

We need this for bug 911257
Whiteboard: [mentor=whimboo][lang=py][good first bug]
Attached patch patch_v1.0 (obsolete) — Splinter Review
I hope this looks good. It's the only approach I had in mind.
Assignee: nobody → cosmin.malutan
Attachment #8401802 - Flags: review?(hskupin)
Comment on attachment 8401802 [details] [diff] [review]
patch_v1.0

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

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +51,5 @@
>                      name = name_map.get(key, key).lower()
>                      self._info['%s_%s' % (filename, name)] = config.has_option(
>                          section, key) and config.get(section, key) or None
> +
> +                cod_name = '%s_%s' % (filename, 'code_name')

I would move this completely out of the outer for loop. At the end of the method you can test if the code name has been set. If not just assign the application name.
Attachment #8401802 - Flags: review?(hskupin) → review-
I don't understand the need for this, can't you just consume the existing application_name when application_code_name is None?
Also, the code name is not mentioned in the documentation, I think it's worth providing an update there. See http://mozbase.readthedocs.org/en/latest/mozversion.html
All that logic should not be repeated by clients. It's really part of this package, which knows best about those files and the contents.
(In reply to Henrik Skupin (:whimboo) from comment #5)
> All that logic should not be repeated by clients. It's really part of this
> package, which knows best about those files and the contents.

Then I still don't understand what we're doing here. You want to return the application name as the value of the code name when there is no code name? Maybe it's just me, but that doesn't sound logical. If the property was something like displayed_name then I think it might make more sense, but code_name sounds much more specific to me.
If you missed the initial implementation of that, here for reference what code name means:

Nightly: Name=Firefox, CodeName=Nightly
Aurora:  Name=Firefox, CodeName=Aurora
Beta:    Name=Firefox
Release: Name=Firefox

Codename has not been added for Beta and Release, because it should only be visible in developer builds. It's used to determine the real name of the application on Windows when you e.g. have to set the default browser. There you see the code name but not the application name.

For details of the implementation see bug 974830. Internally it is the MOZ_APP_DISPLAYNAME, but not sure why it has finally been added wit the name CodeName.
(In reply to Henrik Skupin (:whimboo) from comment #7)
> For details of the implementation see bug 974830. Internally it is the
> MOZ_APP_DISPLAYNAME, but not sure why it has finally been added wit the name
> CodeName.

So I think if mozversion exposed this as application_display_name then it makes sense to fallback to application_name. Having application_code_name to me implies you're asking for something very specific and would potentially want to know when it's not present.
I have no problem with this mapping. So lets just do that as long as no other clients make use of it.
Status: NEW → ASSIGNED
Attached patch patch_v2.0 (obsolete) — Splinter Review
I updated the patch so it will retrieve the "application_display_name" as "application_code_name".
Attachment #8401802 - Attachment is obsolete: true
Attachment #8402623 - Flags: review?(hskupin)
Comment on attachment 8402623 [details] [diff] [review]
patch_v2.0

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

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +57,5 @@
>  
> +        if self._info.has_key('application_name'):
> +            self._info['application_display_name'] = \
> +                self._info.get('application_code_name',
> +                               self._info.get('application_name', None))

What you want is to update the mapping so you store the CodeName directly in application_display_name. And if it doesn't exist you will assign the application_name.

::: testing/mozbase/mozversion/tests/test_binary.py
@@ +70,3 @@
>      def _check_version(self, version):
>          self.assertEqual(version.get('application_name'), 'AppName')
>          self.assertEqual(version.get('application_code_name'), 'AppCodeName')

We don't need that. So replace it with the new property.
Attachment #8402623 - Flags: review?(hskupin) → review-
Attached patch patch_v2.1 (obsolete) — Splinter Review
Henrik I updated the patch as required. Bug 992139
Attachment #8402623 - Attachment is obsolete: true
Attachment #8403957 - Flags: review?(hskupin)
Comment on attachment 8403957 [details] [diff] [review]
patch_v2.1

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

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +54,5 @@
> +
> +                if self._info.has_key('application_name') and not \
> +                    self._info.get('application_display_name'):
> +                    self._info['application_display_name'] = \
> +                        self._info['application_name']

The application name will always be present. There is no need to check that. Also lessen the amount of checks by changing the code to: self._info['application_display_name'] = self._info.get('application_display_name', 'application_name')
Attachment #8403957 - Flags: review?(hskupin) → review-
Attached patch patch_v2.2 (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #13)
> > +                    self._info['application_display_name'] = \
> > +                        self._info['application_name']
> 
> The application name will always be present. There is no need to check that.
> Also lessen the amount of checks by changing the code to:
> self._info['application_display_name'] =
> self._info.get('application_display_name', 'application_name')
get method will return the second argument as it is, so I had to give as second argument:
self._info.get('application_name')
Attachment #8403957 - Attachment is obsolete: true
Attachment #8404657 - Flags: review?(hskupin)
Summary: [mozversion] Enhance mozversion to return "application_name" if "application_code_name" doesn't exists → [mozversion] Set "application_display_name" to "application_name" if CodeName doesn't exist
Comment on attachment 8404657 [details] [diff] [review]
patch_v2.2

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

I'm not sure if you tested the latest changes with release builds of Firefox. It doesn't work.

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +53,5 @@
>                          section, key) and config.get(section, key) or None
> +
> +                self._info['application_display_name'] = \
> +                    self._info.get('application_display_name',
> +                                   self._info.get('application_name'))

Something I have not seen is that if the key is not contained in the ini file, the appropriate field is getting set to None in line 53. So get() will not work, given that this property exists. Sorry for that.
Attachment #8404657 - Flags: review?(hskupin) → review-
Can we get the small update done, Cosmin?
Attached patch patch_v2.3Splinter Review
I updated the patch and tested it, I check if there is "application_display_name" if note I assign the "applciation_name".
Attachment #8404657 - Attachment is obsolete: true
Attachment #8406784 - Flags: review?(hskupin)
Comment on attachment 8406784 [details] [diff] [review]
patch_v2.3

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

Looks fine to me. Lets get also feedback from William before landing.
Attachment #8406784 - Flags: review?(hskupin)
Attachment #8406784 - Flags: review+
Attachment #8406784 - Flags: feedback?(wlachance)
Comment on attachment 8406784 [details] [diff] [review]
patch_v2.3

Perhaps I'm misunderstanding this, but I tend to agree with davehunt's comments above: it looks like you're creating a new abstraction called "display name" inside mozversion, but that's not something that's encapsulated within our app-based metadata.

I would prefer if you just kept mozversion as is and modified the _user_ of it to deal with the case where application_code_name is not present.
Attachment #8406784 - Flags: feedback?(wlachance) → feedback-
William, please see my comment 7 and following. As said there the request was to get something like a display name, but the devs called it CodeName. The value of that key inside of application.ini is used by the system e.g. inside the default application dialog on Windows, to display the name of the application. For releases the application name is used.

Why should every client have to do those checks when it can be encapsulated inside mozversion, and exported as application_display_name?
Comment on attachment 8406784 [details] [diff] [review]
patch_v2.3

Ok, it looks like I misunderstood the discussion. this can go in with my blessing.
Attachment #8406784 - Flags: feedback- → feedback+
https://hg.mozilla.org/integration/mozilla-inbound/rev/368a6f1f7eea

Cosmin, please file a new bug so we can get the version bumped for mozversion.
Target Milestone: --- → mozilla31
https://hg.mozilla.org/mozilla-central/rev/368a6f1f7eea
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.