Closed Bug 1358215 Opened 7 years ago Closed 7 years ago

Add flag to facilitate early landing of photon-animation work ahead of v57

Categories

(Firefox :: Theme, enhancement, P1)

Product:
Firefox
Component:
Theme
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Iteration:
55.5 - May 15
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: sfoster, Assigned: ted)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-animation])

Attachments

(1 file, 1 obsolete file)

bug 1358215 - add MOZ_PHOTON_ANIMATIONS config var/define, default enabled on nightly.
59 bytes, text/x-review-board-request
chmanchester
: review+
Details 
We want to get the photon animation patches landed in nightly before their scheduled release in 57. We'll use a preference and a CSS class on the window root element to occlude these changes while the pref is off.
Flags: qe-verify?
Priority: -- → P2
No need to qe-verify this as verification of this pref will come with verification of other animation features that make use of the pref.
Flags: qe-verify? → qe-verify-
(In reply to Sam Foster [:sfoster] from comment #0)
> We'll use a preference and a CSS class on the
> window root element to occlude these changes while the pref is off.

This would mess with these rules' specificity. Please use ifdef instead.
Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Priority: P2 → P1
As :dao points out, we'll run into selector specificity issues if we use a class on the root element to selectively apply photo-animation -specific rule. The only other option is a %define, and %ifdefs in our CSS, JS etc. There doesnt seem to be a practical way to accomplish what we need at runtime. 

The idea here is that usage will be like: 

# in your mozconfig: 
mk_add_options MOZ_PHOTON_ANIMATIONS=1

# usage
.some rule { original: value }

%ifdef MOZ_PHOTON_ANIMATIONS

.some rule { photon: value }
%endif

This allows us to get this work into the tree and test the aggregate ahead of the 57 release. Once nightly becomes 57, we'll remove the %defines and %ifdefs.
Summary: Add pref to facilitate early landing of photon-animation work ahead of v57 → Add flag to facilitate early landing of photon-animation work ahead of v57
Comment on attachment 8861201 [details]
Bug 1358215 - Add a flag to permit early landing of photon animation changes ahead of 57.

https://reviewboard.mozilla.org/r/133156/#review136002

::: browser/base/moz.build:45
(Diff revision 1)
>  if CONFIG['MOZ_UPDATER']:
>      BROWSER_CHROME_MANIFESTS += ['content/test/appUpdate/browser.ini']
>  
>  DEFINES['MOZ_APP_VERSION'] = CONFIG['MOZ_APP_VERSION']
>  DEFINES['MOZ_APP_VERSION_DISPLAY'] = CONFIG['MOZ_APP_VERSION_DISPLAY']
> +DEFINES['MOZ_PHOTON_ANIMATIONS'] = CONFIG['MOZ_PHOTON_ANIMATIONS']

Please set MOZ_PHOTON_ANIMATIONS to 1 on Nightly builds and 0 otherwise.
Attachment #8861201 - Flags: review?(jaws) → review+
It seems I need to add this new flag to a configure file as well for this to stick. I'm not clear if I should by using set_define or project_flag. And which moz.configure file it should live in? It looks like there maybe a Right Way to do this which I'm not clear on. I'm going to push a new version of the patch - think of it as a straw-man.
Comment on attachment 8861201 [details]
Bug 1358215 - Add a flag to permit early landing of photon animation changes ahead of 57.

https://reviewboard.mozilla.org/r/133156/#review137418
Attachment #8861201 - Flags: review+
Comment on attachment 8861201 [details]
Bug 1358215 - Add a flag to permit early landing of photon animation changes ahead of 57.

https://reviewboard.mozilla.org/r/133156/#review137420

This needs some changes, but I think you can get what you want with very little work. I'm going to be PTO tomorrow so if you want a followup review before next week ping another build peer (glandium is on vacation, so chmanchester, gps, or mshal are all good choices).

::: browser/base/moz.build:47
(Diff revision 3)
>  
>  DEFINES['MOZ_APP_VERSION'] = CONFIG['MOZ_APP_VERSION']
>  DEFINES['MOZ_APP_VERSION_DISPLAY'] = CONFIG['MOZ_APP_VERSION_DISPLAY']
>  
> +# photon animations: Nightly-only and unless explicitly disabled
> +if (CONFIG['NIGHTLY_BUILD'] and not (CONFIG['MOZ_PHOTON_ANIMATIONS'] is 0)) :

kinda nit-picky, but the convention we use for `CONFIG` entries is just to treat them as boolean, since they're actually either `1` or unset, so this should just be:
```
if CONFIG['NIGHTLY_BUILD'] and CONFIG['MOZ_PHOTON_ANIMATIONS']:
```

...but really you should make `MOZ_PHOTON_ANIMATIONS` get set automatically for `NIGHTLY_BUILD`, I'll cover this below.

::: browser/moz.configure:12
(Diff revision 3)
>  imply_option('MOZ_PLACES', True)
>  imply_option('MOZ_SERVICES_HEALTHREPORT', True)
>  imply_option('MOZ_SERVICES_SYNC', True)
>  imply_option('MOZ_SERVICES_CLOUDSYNC', True)
>  
> +set_define('MOZ_PHOTON_ANIMATIONS', False)

So:
a) `set_define` is equivalent to setting something in `DEFINES`, but globally. If you only need the define in one place just use `DEFINES` in moz.build like you're doing above. If you need it globally then you'll want to keep this, but...
b) You need to define an option somehow. The simplest way is to use `project_flag`, like: https://dxr.mozilla.org/mozilla-central/rev/0b77ed3f26c5335503bc16e85b8c067382e7bb1e/toolkit/moz.configure#507 . That even has a `set_as_define` option if you want the define as well. Look at `MOZ_ANDROID_CUSTOM_TABS` for an example that defaults to true for nightly only: https://dxr.mozilla.org/mozilla-central/source/mobile/android/moz.configure#42 .
Attachment #8861201 - Flags: review-
> b) You need to define an option somehow. The simplest way is to use
> `project_flag`, like:
> https://dxr.mozilla.org/mozilla-central/rev/
> 0b77ed3f26c5335503bc16e85b8c067382e7bb1e/toolkit/moz.configure#507 . That
> even has a `set_as_define` option if you want the define as well. Look at
> `MOZ_ANDROID_CUSTOM_TABS` for an example that defaults to true for nightly
> only:
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/moz.
> configure#42 .

Thanks for the detailed answer here. I'm going to push an updated patch using project_flag. The one thing I'm not clear on is how I'll tell people to disable this option for their nightly builds. I was thinking one would just mk_add_options MOZ_PHOTON_ANIMATIONS=0 to the mozconfig file, or even just use an env. variable when building: MOZ_PHOTON_ANIMATIONS=0 mach build, but it looks like I'll need to add an imply_option('MOZ_PHOTON_ANIMATIONS', False) somewhere?
Attachment #8861201 - Flags: review?(mh+mozilla) → review?(gps)
Comment on attachment 8861201 [details]
Bug 1358215 - Add a flag to permit early landing of photon animation changes ahead of 57.

Going back to ted since he did initial review.
Attachment #8861201 - Flags: review?(gps) → review?(ted)
Iteration: 55.4 - May 1 → 55.5 - May 15
(In reply to Sam Foster [:sfoster] from comment #11)
> > b) You need to define an option somehow. The simplest way is to use
> > `project_flag`, like:
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 0b77ed3f26c5335503bc16e85b8c067382e7bb1e/toolkit/moz.configure#507 . That
> > even has a `set_as_define` option if you want the define as well. Look at
> > `MOZ_ANDROID_CUSTOM_TABS` for an example that defaults to true for nightly
> > only:
> > https://dxr.mozilla.org/mozilla-central/source/mobile/android/moz.
> > configure#42 .
> 
> Thanks for the detailed answer here. I'm going to push an updated patch
> using project_flag. The one thing I'm not clear on is how I'll tell people
> to disable this option for their nightly builds. I was thinking one would
> just mk_add_options MOZ_PHOTON_ANIMATIONS=0 to the mozconfig file, or even
> just use an env. variable when building: MOZ_PHOTON_ANIMATIONS=0 mach build,

MOZ_PHOTON_ANIMATIONS=
> > The one thing I'm not clear on is how I'll tell people
> > to disable this option for their nightly builds. I was thinking one would
> > just mk_add_options MOZ_PHOTON_ANIMATIONS=0 to the mozconfig file, or even
> > just use an env. variable when building: MOZ_PHOTON_ANIMATIONS=0 mach build,
> 
> MOZ_PHOTON_ANIMATIONS=

This produces an error: 

mozbuild.configure.options.InvalidOptionError: MOZ_PHOTON_ANIMATIONS= can not be set by environment. Values are accepted from: implied 

Am I doing this wrong?
Flags: needinfo?(mh+mozilla)
Oh, ugh. Per bug 1257326 comment 12, this was an explicit design decision of `project_flag`. :-/
Comment on attachment 8861201 [details]
Bug 1358215 - Add a flag to permit early landing of photon animation changes ahead of 57.

https://reviewboard.mozilla.org/r/133156/#review140270

Apparently I gave sfoster bad advice, so I'm just going to write the patch he needs.
Attachment #8861201 - Flags: review?(ted)
Assignee: sfoster → ted
Attachment #8861201 - Attachment is obsolete: true
Iteration: 55.5 - May 15 → ---
We'll need this in browser code too, so I suspect toolkit/moz.configure is not the right place for this.

While you're at it, would you mind adding MOZ_PHOTON_THEME too for the work under bug 1325171?
> We'll need this in browser code too, so I suspect toolkit/moz.configure is not the right place for this.

Don't worry, things defined in any part of configure are available for the full build.

> While you're at it, would you mind adding MOZ_PHOTON_THEME too for the work under bug 1325171?

I don't know what's needed there, but if it's just a simple build option/define like this then it should be easy enough to do.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8865601 [details]
bug 1358215 - add MOZ_PHOTON_ANIMATIONS config var/define, default enabled on nightly.

https://reviewboard.mozilla.org/r/137222/#review140276
Attachment #8865601 - Flags: review?(cmanchester) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #20)
> > We'll need this in browser code too, so I suspect toolkit/moz.configure is not the right place for this.
> 
> Don't worry, things defined in any part of configure are available for the
> full build.

Note that's not entirely true, but toolkit is the right place for anything Gecko.
(In reply to Dão Gottwald [::dao] from comment #19)
> While you're at it, would you mind adding MOZ_PHOTON_THEME too for the work
> under bug 1325171?

Since this patch already has r+ I'm going to punt that to a separate bug.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #16)
> Oh, ugh. Per bug 1257326 comment 12, this was an explicit design decision of
> `project_flag`. :-/

I filed bug 1363357 on adding something to moz.configure that would have been useful here.
Pushed by tmielczarek@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5914a5dcc385
add MOZ_PHOTON_ANIMATIONS config var/define, default enabled on nightly. r=chmanchester
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #23)
> Since this patch already has r+ I'm going to punt that to a separate bug.

Filed bug 1363358 for MOZ_PHOTON_THEME.
Backed out for bustage / failing python/mozbuild/mozbuild/test/configure/lint.py:

https://hg.mozilla.org/integration/autoland/rev/d813ff0d21b5baecd7f0b827973e3fe98d312942

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5914a5dcc3852742e75d0aa988e45f7dac26b284&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=97668827&repo=autoland

[task 2017-05-09T13:45:15.543236Z] 13:45:15     INFO - TEST-PASS | /home/worker/workspace/build/src/python/mozbuild/mozbuild/test/test_base.py | TestPathArgument.test_path_argument
[task 2017-05-09T13:45:15.543259Z] 13:45:15     INFO - Wrote mozconfig /tmp/tmpIy0mrg/mozconfig
[task 2017-05-09T13:45:15.602127Z] 13:45:15     INFO - /home/worker/workspace/build/src/python/mozbuild/mozbuild/test/configure/lint.py
[task 2017-05-09T13:45:15.602409Z] 13:45:15  WARNING - TEST-UNEXPECTED-FAIL | /home/worker/workspace/build/src/python/mozbuild/mozbuild/test/configure/lint.py | Lint.test_browser, line 26: Missing @depends for `result`: '--help'
[task 2017-05-09T13:45:15.602845Z] 13:45:15     INFO - ERROR: test_browser (__main__.Lint)
[task 2017-05-09T13:45:15.606387Z] 13:45:15     INFO - Traceback (most recent call last):
[task 2017-05-09T13:45:15.606444Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/test/configure/lint.py", line 26, in test
[task 2017-05-09T13:45:15.606469Z] 13:45:15     INFO -     return func(self, project)
[task 2017-05-09T13:45:15.607302Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/test/configure/lint.py", line 58, in lint
[task 2017-05-09T13:45:15.607944Z] 13:45:15     INFO -     sandbox.run(os.path.join(topsrcdir, 'moz.configure'))
[task 2017-05-09T13:45:15.608559Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/configure/lint.py", line 34, in run
[task 2017-05-09T13:45:15.608615Z] 13:45:15     INFO -     self.include_file(path)
[task 2017-05-09T13:45:15.608841Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/configure/__init__.py", line 343, in include_file
[task 2017-05-09T13:45:15.609027Z] 13:45:15     INFO -     exec_(code, self)
[task 2017-05-09T13:45:15.609248Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/util.py", line 59, in exec_
[task 2017-05-09T13:45:15.613404Z] 13:45:15     INFO -     exec(object, globals, locals)
[task 2017-05-09T13:45:15.613471Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/moz.configure", line 134, in <module>
[task 2017-05-09T13:45:15.613517Z] 13:45:15     INFO -     include(include_project_configure)
[task 2017-05-09T13:45:15.613578Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/configure/__init__.py", line 647, in include_impl
[task 2017-05-09T13:45:15.613614Z] 13:45:15     INFO -     self.include_file(what)
[task 2017-05-09T13:45:15.613678Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/configure/__init__.py", line 343, in include_file
[task 2017-05-09T13:45:15.613720Z] 13:45:15     INFO -     exec_(code, self)
[task 2017-05-09T13:45:15.613777Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/util.py", line 59, in exec_
[task 2017-05-09T13:45:15.613818Z] 13:45:15     INFO -     exec(object, globals, locals)
[task 2017-05-09T13:45:15.613872Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/browser/moz.configure", line 11, in <module>
[task 2017-05-09T13:45:15.613914Z] 13:45:15     INFO -     include('../toolkit/moz.configure')
[task 2017-05-09T13:45:15.613980Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/configure/__init__.py", line 647, in include_impl
[task 2017-05-09T13:45:15.614017Z] 13:45:15     INFO -     self.include_file(what)
[task 2017-05-09T13:45:15.614080Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/configure/__init__.py", line 343, in include_file
[task 2017-05-09T13:45:15.614120Z] 13:45:15     INFO -     exec_(code, self)
[task 2017-05-09T13:45:15.614180Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/util.py", line 59, in exec_
[task 2017-05-09T13:45:15.614223Z] 13:45:15     INFO -     exec(object, globals, locals)
[task 2017-05-09T13:45:15.614281Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/toolkit/moz.configure", line 543, in <module>
[task 2017-05-09T13:45:15.614328Z] 13:45:15     INFO -     default=delayed_getattr(milestone, 'is_nightly'))
[task 2017-05-09T13:45:15.614392Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/configure/lint.py", line 118, in option_impl
[task 2017-05-09T13:45:15.614442Z] 13:45:15     INFO -     result = super(LintSandbox, self).option_impl(*args, **kwargs)
[task 2017-05-09T13:45:15.614507Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/configure/__init__.py", line 564, in option_impl
[task 2017-05-09T13:45:15.614558Z] 13:45:15     INFO -     kwargs = {k: self._resolve(v) for k, v in kwargs.iteritems()
[task 2017-05-09T13:45:15.614621Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/configure/__init__.py", line 565, in <dictcomp>
[task 2017-05-09T13:45:15.614658Z] 13:45:15     INFO -     if k != 'when'}
[task 2017-05-09T13:45:15.614719Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/configure/__init__.py", line 417, in _resolve
[task 2017-05-09T13:45:15.614755Z] 13:45:15     INFO -     need_help_dependency)
[task 2017-05-09T13:45:15.614812Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/util.py", line 925, in method_call
[task 2017-05-09T13:45:15.614857Z] 13:45:15     INFO -     cache[args] = self.func(instance, *args)
[task 2017-05-09T13:45:15.614920Z] 13:45:15     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/configure/lint.py", line 113, in _value_for_depends
[task 2017-05-09T13:45:15.614956Z] 13:45:15     INFO -     obj.name)
[task 2017-05-09T13:45:15.615001Z] 13:45:15     INFO - ConfigureError: Missing @depends for `result`: '--help'
Flags: needinfo?(ted)
I can't say I really *understand* the lint test failure there, but the thing it's complaining about is the function defined inside `delayed_getattr`. I changed the patch to add a separate `is_nightly` function and use that, and it passes the lint check now.
Flags: needinfo?(ted)
chmanchester gave r+ over IRC for the updated patch, and I filed bug 1363811 to find out if the lint test is doing the right thing.
Pushed by tmielczarek@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17670f6aeeae
add MOZ_PHOTON_ANIMATIONS config var/define, default enabled on nightly. r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/17670f6aeeae11dde7258edecf1373829fca9979
bug 1358215 - add MOZ_PHOTON_ANIMATIONS config var/define, default enabled on nightly. r=chmanchester
https://hg.mozilla.org/mozilla-central/rev/17670f6aeeae
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.5 - May 15
Blocks: 1367166
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: