1386207 - Drop OS_* defines under toolkit/

Status: NEW

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

Description

[Jan Beich]

+++ 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 1

[Jan Beich]

Attachment: Bug 1386207 - Don't use Chromium platform macros under toolkit/.

Review commit: https://reviewboard.mozilla.org/r/163380/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163380/

Comment 2

[Jan Beich]

Review hint: jwatt to check changes specific to bug 1350641, glandium - the rest given familarity with ipc/chromium/ and the build system.

Comment 3

[Jan Beich]

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

Comment 4

[Mike Hommey [:glandium]]

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.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

(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?

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Let's just land this.

Comment 7

[[Back July 11] Ryan VanderMeulen]

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Comment 8

[Jan Beich]

Comment on attachment 8892419 [details]
Bug 1386207 - Don't use Chromium platform macros under toolkit/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163380/diff/1-2/

Comment 9

[Jan Beich]

Addressed churn issue raised by :glandium and rebased against bug 1419959 comment 7.

Comment 10

[Mike Hommey [:glandium]]

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.

Comment 11

[Jonathan Watt [:jwatt]]

(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.

Comment 12

[Jan Beich]

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

[Mike Hommey [:glandium]]

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.

Comment 14

[Jan Beich]

(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

[Mike Hommey [:glandium]]

That's just diff being capricious. Doesn't mean anything.

Comment 16

[Jan Beich]

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

[Mike Hommey [:glandium]]

Either way blame will attribute you as having changed one of those returns. Just keep the code in the order it currently has.

Comment 18

[Jan Beich]

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