Open
Bug 1386207
Opened 8 years ago
Updated 3 years ago
Drop OS_* defines under toolkit/
Categories
(Toolkit :: Startup and Profile System, enhancement, P3)
Toolkit
Startup and Profile System
Tracking
()
NEW
Tracking | Status | |
---|---|---|
firefox57 | --- | fix-optional |
People
(Reporter: jbeich, Unassigned)
References
Details
Attachments
(1 file)
+++ This bug was initially created as a clone of Bug #1386200 +++
Gecko provides XP_<PLATFORM> while Blink provides OS_<PLATFORM>. Gecko can also use the latter but it's both discouraged and can be confusing if both styles are used in the same file.
Comment hidden (mozreview-request) |
Review hint: jwatt to check changes specific to bug 1350641, glandium - the rest given familarity with ipc/chromium/ and the build system.
Comment on attachment 8892419 [details]
Bug 1386207 - Don't use Chromium platform macros under toolkit/.
https://reviewboard.mozilla.org/r/163380/#review168736
::: toolkit/xre/nsEmbedFunctions.cpp:339
(Diff revision 1)
> -#ifdef OS_POSIX
> +#if defined(XP_WIN)
> - return 30; // seconds
> -#elif defined(OS_WIN)
> return 10000; // milliseconds
> #else
> - return 0;
> + return 30; // seconds
After bug 969757 everything but Windows is Unix. No need for an ad-hoc condition for what isn't supported even as Tier3 platform.
http://searchfox.org/mozilla-central/rev/09c065976fd4/build/moz.configure/init.configure#683
http://searchfox.org/mozilla-central/rev/09c065976fd4/ipc/chromium/chromium-config.mozbuild#38
![]() |
||
Updated•8 years ago
|
Attachment #8892419 -
Flags: review?(jwatt) → review+
Comment 4•8 years ago
|
||
mozreview-review |
Comment on attachment 8892419 [details]
Bug 1386207 - Don't use Chromium platform macros under toolkit/.
https://reviewboard.mozilla.org/r/163380/#review171602
::: toolkit/xre/nsEmbedFunctions.cpp:538
(Diff revision 1)
> #ifdef MOZ_WIDGET_GTK
> // Setting the name here avoids the need to pass this through to gtk_init().
> g_set_prgname(aArgv[0]);
> #endif
>
> -#ifdef OS_POSIX
> +#if defined(XP_WIN)
You could get away with less code moving if you turned ifdef OS_POSIX into ifndef XP_WIN.
Attachment #8892419 -
Flags: review?(mh+mozilla) → review+
Comment 5•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #4)
> You could get away with less code moving if you turned ifdef OS_POSIX into
> ifndef XP_WIN.
Are you planning to implement the suggested change above or is this ready to land?
Updated•8 years ago
|
Assignee: nobody → jbeich
Comment 7•8 years ago
|
||
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Addressed churn issue raised by :glandium and rebased against bug 1419959 comment 7.
Flags: needinfo?(jbeich)
Attachment #8892419 -
Flags: review+ → review?(mh+mozilla)
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8892419 [details]
Bug 1386207 - Don't use Chromium platform macros under toolkit/.
https://reviewboard.mozilla.org/r/163380/#review208822
::: toolkit/xre/nsEmbedFunctions.cpp:328
(Diff revision 2)
> pause *= 1000; // convert to ms
> #endif
> return pause;
> }
> }
> -#ifdef OS_POSIX
> +#if defined(XP_WIN)
You /could/ make this a ifndef and keep the current order.
Attachment #8892419 -
Flags: review?(mh+mozilla) → review+
![]() |
||
Comment 11•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #10)
> You /could/ make this a ifndef and keep the current order.
I'd prefer that too, FWIW.
![]() |
||
Updated•8 years ago
|
Attachment #8892419 -
Flags: review?(jwatt) → review+
Reporter | ||
Comment 12•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8892419 [details]
Bug 1386207 - Don't use Chromium platform macros under toolkit/.
https://reviewboard.mozilla.org/r/163380/#review208822
> You /could/ make this a ifndef and keep the current order.
That wouldn't reduce churn. `#elif defined(OS_WIN)` -> `#else` change still requires to move one more line. I hope you're not proposing the following:
```
#if !defined(XP_WIN)
...
#elif defined(XP_WIN)
...
#else
...
#endif
```
Comment 13•8 years ago
|
||
The change requires to *remove* lines, not move them. That #else was never taken, and the #else you're mentioning wouldn't be taken either.
Reporter | ||
Comment 14•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #13)
> That #else was never taken, and the #else you're mentioning wouldn't be taken either.
#else not but the line under it was. Here's an illustration:
Before
-#ifdef OS_POSIX
- return 30; // seconds
-#elif defined(OS_WIN)
+#if defined(XP_WIN)
return 10000; // milliseconds
#else
- return 0;
+ return 30; // seconds
#endif
After
-#ifdef OS_POSIX
+#if !defined(XP_WIN)
return 30; // seconds
-#elif defined(OS_WIN)
- return 10000; // milliseconds
#else
- return 0;
+ return 10000; // milliseconds
#endif
Comment 15•8 years ago
|
||
That's just diff being capricious. Doesn't mean anything.
Reporter | ||
Comment 16•8 years ago
|
||
It affects at least git blame. After your proposed change I'm listed as the one who last modified the line after #else.
Comment 17•8 years ago
|
||
Either way blame will attribute you as having changed one of those returns. Just keep the code in the order it currently has.
Reporter | ||
Comment 18•8 years ago
|
||
Which is the reverse of the order of the #ifdef just above it. How an inconsistent order improves readability?
https://searchfox.org/mozilla-central/rev/be78e6ea9b10/toolkit/xre/nsEmbedFunctions.cpp#322
Updated•3 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•