User Since

Jul 13 2018, 5:18 PM (413 w, 5 d)

Availability

Available

Review Queue

6

Requesting changes as per @Itiel's comments. Thanks!

Nice, thank you!

I tried removing , and it seemed to work. We redid the some related CSS recently, so maybe it's not necessary anymore. I don't want to complicate this patch with that though.

fix smartbar leak

fixes for test failures

lgtm, thanks!

In D304697#10608331, @mbeier wrote:

in child processes could be a that tunnels function calls through the actor to the parent controller. There could be a whitelist of all parent controller functions that support being called over IPC (they have to be async and have serializable parameters and return value). In the parent process, would still be a direct reference to the parent controller.
That way, instances in child processes would have access to the IPC enabled subset of the parent controller interface while instances in the parent process can easily access the whole parent controller. We also wouldn't have to create actors for moz-urlbars in the parent process.

In TypeScript, you could do something like

parentController: UrlbarParentController | UrlbarParentControllerIPCSubset

// If this returns true, TS knows that the full parent controller is available
inParent(): this is this & { parentController: UrlbarParentController } {
 return ... 
}

So we could statically check that no parent-only methods are used in a child process. We would probably also have to do the same thing but in reverse for the parent controller.

What do you think about this? Did you already have another idea how the actor communication between child and parent could look like?

The suggestion doesn't really affect the work in this patch, except that the patch creates actors even in the parent process (easy to change later). With the comments addressed, this patch looks good to me.

address review comments

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of , , , , . Tip: this Firefox add-on makes it easy!

https://treeherder.mozilla.org/jobs?repo=try&revision=1c0f3c1225d05199e2d5dcb44e841a331f19c1f3

rebased

Let's get additional review from Moritz on this and then we can decide if we're comfortable enough or want an additional review from Marco or Mark.

Marking @mstriemer as blocking for recomp while I'm gonna ask a questions on behalf of desktop-theme-reviewers, and requesting changes on that for visibility:

The changes are new since I looked at this, so I'm tagging firefox-desktop-core-reviewers for an additional review.

Thanks!

The default button style is still not fixed. At this point, let's take this.

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of , , , , . Tip: this Firefox add-on makes it easy!

In D301961#10511905, @sthompson wrote:

add-circle-fill and subtract-circle-fill

The Proton versions of these icons draw a and then a on top. The Nova versions in acorn-icons draw a single describing the filled area around the + or -. That would mean the Nova version could only be used when the icon is supposed to have a transparent + or -.

Since there are existing uses for explicitly coloring the + or - (e.g. the application menu zoom controls), I manually updated the Nova version of this icons to draw a and then remove the circle drawing parts of the . This means the Nova versions in this patch diverge from the acorn-icons repo at this time. I'm talking with the icon designers about how they want to proceed, but I think in practical terms, we need these icons to work the same way as Proton.

And I think it would be fine to give them all hardcoded sizes according to the spec rather than basing their heights on the height of the urlbar.

In D296488#10433240, @Gijs wrote:

I defer to @dao here but I'm not sure I understand the benefit of all the custom prefixes?

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of , , , , . Tip: this Firefox add-on makes it easy!

Thanks!

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of , , , , . Tip: this Firefox add-on makes it easy!

I haven't yet heard back from @Juliana or @jules. I think I still want to veto this because it feels like a semantically confusing hack on top of a suboptimal default button style.