Open
Bug 1386207
Opened 7 years ago
Updated 2 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•7 years ago
|
Attachment #8892419 -
Flags: review?(jwatt) → review+
Comment 4•7 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•7 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•7 years ago
|
Assignee: nobody → jbeich
Comment 7•7 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•7 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•7 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•7 years ago
|
Attachment #8892419 -
Flags: review?(jwatt) → review+
Reporter | ||
Comment 12•7 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•7 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•7 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•7 years ago
|
||
That's just diff being capricious. Doesn't mean anything.
Reporter | ||
Comment 16•7 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•7 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•7 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•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•