Support synchonizing BrowsingContext fields between IPC processes

ASSIGNED
Assigned to

Status

()

P3
normal
ASSIGNED
19 days ago
3 days ago

People

(Reporter: farre, Assigned: farre)

Tracking

(Blocks: 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Fission Milestone:M1)

Details

(Whiteboard: [2/14] patch ETA 2/18)

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

19 days ago
Blocks: 1519151
(Assignee)

Updated

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

Updated

19 days ago
Blocks: 1519151
(Assignee)

Updated

18 days ago
Summary: Support synchonizing BrowsingContext fields over IPC between processes → Support synchonizing BrowsingContext fields between IPC processes

Updated

18 days ago
Fission Milestone: --- → M1
(Assignee)

Comment 1

12 days ago

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

What's here:

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

Updated

10 days ago
Depends on: 1525866

Comment 2

10 days ago
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

10 days 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

4 days ago
Depends on: 1525887
Whiteboard: [2/14] patch ETA 2/18
You need to log in before you can comment on or make changes to this bug.