Open Bug 1386207 Opened 2 years ago Updated 2 years ago

Drop OS_* defines under toolkit/

Categories

(Toolkit :: Startup and Profile System, enhancement, P3)

enhancement

Tracking

()

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.
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
Attachment #8892419 - Flags: review?(jwatt) → 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+
(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?
Flags: needinfo?(jbeich)
Priority: -- → P3
Let's just land this.
Keywords: checkin-needed
Assignee: nobody → jbeich
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
Addressed churn issue raised by :glandium and rebased against bug 1419959 comment 7.
Flags: needinfo?(jbeich)
Attachment #8892419 - Flags: review+ → review?(mh+mozilla)
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+
(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.
Attachment #8892419 - Flags: review?(jwatt) → review+
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
```
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.
(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
That's just diff being capricious. Doesn't mean anything.
It affects at least git blame. After your proposed change I'm listed as the one who last modified the line after #else.
Either way blame will attribute you as having changed one of those returns. Just keep the code in the order it currently has.
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
Assignee: jbeich → nobody
You need to log in before you can comment on or make changes to this bug.