Support synchonizing BrowsingContext fields between IPC processes

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P1
normal
RESOLVED FIXED
3 months ago
a month ago

People

(Reporter: farre, Assigned: farre)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Fission Milestone:M1, firefox67 fixed)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Updated

3 months ago
Blocks: 1519151
(Assignee)

Updated

3 months ago
Assignee: nobody → afarre
No longer blocks: 1519151
Status: NEW → ASSIGNED
Priority: -- → P3
(Assignee)

Updated

3 months ago
Blocks: 1519151
(Assignee)

Updated

3 months ago
Summary: Support synchonizing BrowsingContext fields over IPC between processes → Support synchonizing BrowsingContext fields between IPC processes

Updated

3 months ago
Fission Milestone: --- → M1
(Assignee)

Comment 1

3 months ago

What's here:

  • Transaction class
  • Macros for declaring synced fields
  • IPDLParamTrait for Transaction
Attachment #9041439 - Flags: feedback?(nika)
(Assignee)

Updated

3 months ago
Depends on: 1525866
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?
Attachment #9041439 - Flags: feedback?(nika) → feedback+
(Assignee)

Comment 3

3 months ago

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

(Assignee)

Updated

2 months ago
Depends on: 1525887

Updated

2 months ago
Whiteboard: [2/14] patch ETA 2/18

Updated

2 months ago
Priority: P3 → P1
Whiteboard: [2/14] patch ETA 2/18 → [2/19] patch under review
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1490303

Updated

2 months ago
Whiteboard: [2/19] patch under review → [2/21] patch r+, ready to land

Comment 7

2 months ago
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f830047054e5
Add transactions for syncing BrowsingContext fields. r=nika

Comment 8

2 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

This is awesome! \o/

Updated

2 months ago
Whiteboard: [2/21] patch r+, ready to land

Updated

2 months ago
Blocks: 1530550
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.