1523645 - Support synchonizing BrowsingContext fields between IPC processes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P1)

Comment 1

[Andreas Farre [:farre]]

Attachment: 0001-wip.patch

What's here:

• Transaction class
• Macros for declaring synced fields
• IPDLParamTrait for Transaction

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9041439 [details] [diff] [review]
0001-wip.patch

Review of attachment 9041439 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/BrowsingContext.h
@@ +38,5 @@
>  class Sequence;
>  struct WindowPostMessageOptions;
>  class WindowProxyHolder;
>  
> +#define FOR_EACH_SYNCED_FIELD(fn_, ...)         \

what's the idea behind the ... here?

@@ +42,5 @@
> +#define FOR_EACH_SYNCED_FIELD(fn_, ...)         \
> +  fn_(Name, nsString)
> +
> +#define GETTER(name, type) const type& Get ## name () const { return m ## name ; }
> +#define MEMBER(name, type, ...) type m ## name;

Let's not use such generic names for these macros. These are in headers and are at risk of conflicting with other macros.

Also we can use `NAME(name)` in the other places where you write `m ## name` :-)

@@ +43,5 @@
> +  fn_(Name, nsString)
> +
> +#define GETTER(name, type) const type& Get ## name () const { return m ## name ; }
> +#define MEMBER(name, type, ...) type m ## name;
> +#define MAYBE_MEMBER(name, type) mozilla::Maybe< type > m ## name;

Where are you using MAYBE_MEMBER?

@@ +47,5 @@
> +#define MAYBE_MEMBER(name, type) mozilla::Maybe< type > m ## name;
> +#define MAYBE_TYPE(_, type) mozilla::Maybe< type >
> +#define NAME(name, _) m ## name
> +
> +#define SYNCED_FIELDS(FOR_EACH_FIELD)                                     \

You can probably skip having `FOR_EACH_FIELD` be an argument. You can just pass in `FOR_EACH_SYNCED_FIELD`.

@@ +55,5 @@
> +   public:                                                                \
> +    typedef Tuple<FOR_EACH_FIELD(MAYBE_TYPE, ,)> Type;                    \
> +    explicit Transaction(uint64_t aBrowsingContextId)                     \
> +        : mBrowsingContextId(aBrowsingContextId) {}                       \
> +    NS_INLINE_DECL_REFCOUNTING(Transaction)                               \

Do we need this to be refcounted?

@@ +56,5 @@
> +    typedef Tuple<FOR_EACH_FIELD(MAYBE_TYPE, ,)> Type;                    \
> +    explicit Transaction(uint64_t aBrowsingContextId)                     \
> +        : mBrowsingContextId(aBrowsingContextId) {}                       \
> +    NS_INLINE_DECL_REFCOUNTING(Transaction)                               \
> +    auto Tie() { return mozilla::Tie(FOR_EACH_FIELD(NAME, , )); }         \

Why the extra commas?

Comment 3

[Andreas Farre [:farre]]

(In reply to :Nika Layzell from comment #2)

Comment on attachment 9041439 [details] [diff] [review]
0001-wip.patch

Review of attachment 9041439 [details] [diff] [review]:

::: docshell/base/BrowsingContext.h
@@ +38,5 @@

class Sequence;
struct WindowPostMessageOptions;
class WindowProxyHolder;

+#define FOR_EACH_SYNCED_FIELD(fn_, ...) \

what's the idea behind the ... here?

When FOR_EACH_SYNCED_FIELD expands to more fields I need to be able to handle to behaviours:

1. The passed macro handles its own separator, as for the statement case.
2. Interspersed separators, as for type packs and arguments

@@ +42,5 @@

+#define FOR_EACH_SYNCED_FIELD(fn_, ...) \

• fn_(Name, nsString)

+#define GETTER(name, type) const type& Get ## name () const { return m ## name ; }
+#define MEMBER(name, type, ...) type m ## name;

Let's not use such generic names for these macros. These are in headers and
are at risk of conflicting with other macros.

Sure thing.

Also we can use NAME(name) in the other places where you write m ## name
:-)

@@ +43,5 @@

• fn_(Name, nsString)

+#define GETTER(name, type) const type& Get ## name () const { return m ## name ; }
+#define MEMBER(name, type, ...) type m ## name;
+#define MAYBE_MEMBER(name, type) mozilla::Maybe< type > m ## name;

Where are you using MAYBE_MEMBER?

Just above the 'private:' in Transaction.

@@ +47,5 @@

+#define MAYBE_MEMBER(name, type) mozilla::Maybe< type > m ## name;
+#define MAYBE_TYPE(_, type) mozilla::Maybe< type >
+#define NAME(name, _) m ## name
+
+#define SYNCED_FIELDS(FOR_EACH_FIELD) \

You can probably skip having FOR_EACH_FIELD be an argument. You can just
pass in FOR_EACH_SYNCED_FIELD.

Right.

@@ +55,5 @@

• public: \
• typedef Tuple<FOR_EACH_FIELD(MAYBE_TYPE, ,)> Type; \
• explicit Transaction(uint64_t aBrowsingContextId) \

•    : mBrowsingContextId(aBrowsingContextId) {}                       \
  

• NS_INLINE_DECL_REFCOUNTING(Transaction) \

Do we need this to be refcounted?

No.

@@ +56,5 @@

• typedef Tuple<FOR_EACH_FIELD(MAYBE_TYPE, ,)> Type; \
• explicit Transaction(uint64_t aBrowsingContextId) \

•    : mBrowsingContextId(aBrowsingContextId) {}                       \
  

• NS_INLINE_DECL_REFCOUNTING(Transaction) \
• auto Tie() { return mozilla::Tie(FOR_EACH_FIELD(NAME, , )); } \

Why the extra commas?

So, that's what the varargs in the macro is used for. Tie is variadic, and I use FOR_EACH_FIELD to intersperse commas.

Comment 4

[Andreas Farre [:farre]]

Attachment: Bug 1523645 - Add transactions for syncing BrowsingContext fields.

Comment 5

[Andreas Farre [:farre]]

Try is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=da6dbcc119a8bf96965ff9cc0281bff634e0b736&group_state=expanded

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f830047054e5
Add transactions for syncing BrowsingContext fields. r=nika

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/f830047054e5

Comment 9

[:Felipe Gomes (needinfo for replies!)]

This is awesome! \o/