336288 - integrate UI portion of safebrowsing into /browser/components/safebrowsing

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Tony Chang (Google)]

The UI portions of the safebrowsing extension need to be moved to /browser/components/safebrowsing/.

Comment 1

[Tony Chang (Google)]

Attachment: adding ui components to browser/components

Ports UI elements of code from /extensions/safebrowsing/ into /browser/components/safebrowsing.  Branding code has been removed.  Images will be attached separately to the bug.

This also includes some small changes to /toolkit/components/url-classifier (namely, removing globalstore.js and putting it in /browser/... instead as mentioned in bug 335564).

Comment 2

[Tony Chang (Google)]

Attachment: X to close the warning bubble

Comment 3

[Tony Chang (Google)]

Attachment: page overlay

Comment 4

[Tony Chang (Google)]

Attachment: the tail on the warning bubble

Comment 5

[Tony Chang (Google)]

Attachment: urlbar icon

Comment 6

[Tony Chang (Google)]

Attachment: larger version of urlbar icon shown next to warning text

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 220588 [details] [diff] [review]
adding ui components to browser/components

Here's the way I'm going to approach this in the interests of getting the most out of the time available. Rather than go into a lengthy review cycle (I have a bunch of comments), I'm going to grant r= for the integration hooks ifdef'ed out as they are, and also give branch approval. Then I can follow up afterwards with a detailed commentary and we can iterate. 

So, to clarify:

r+a=[e-mail] for the code additions to browser etc. where the #ifdef MOZ_SAFE_BROWSING sections are included. You can check in that and all of the code controlled by that to the trunk and branch. I will provide a more detailed review of this code in the coming days. The changes I request can be handled as followon patches to this bug.

Comment 8

[Tony Chang (Google)]

Sounds good.  All the code should be #ifdef'ed by MOZ_SAFE_BROWSING, so I'll have it committed.

Comment 9

[Brett Wilson]

Attachment: Patch to check in

Comment 10

[Tony Chang (Google)]

(In reply to comment #9)
> Created an attachment (id=220669) [edit]
> Patch to check in
> 

The png files should be copied into the pinstripe theme dir as well.

Comment 11

[Brett Wilson]

On branch. Leaving open for trunk checkin.

Comment 12

[Bryan]

(In reply to comment #2)
> Created an attachment (id=220589) [edit]
> X to close the warning bubble
> 
This doesn't look consistent with the other Winstripe [X] close buttons.  Can we fix this to use the default [X] included in Winstripe?

~B

Comment 13

[Brett Wilson]

(In reply to comment #12)
> This doesn't look consistent with the other Winstripe [X] close buttons.  Can
> we fix this to use the default [X] included in Winstripe?

I imagine they will all get replaced when the new theme is done.

Comment 14

[Brett Wilson]

On trunk. Hopefully I didn't forget anything.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Please don't forget to cvs-remove any now-unused parts of what's in extensions/safe-browsing/ that have been copied into browser/.

Comment 16

[John Mellor (Jomel)]

Looking through the code I saw you've hooked up a custom tab switch listener:

// There's no tabswitch event in Firefox, so we fake it by watching
// for selects on the tabbox.
this.onTabSwitchClosure_ = BindToObject(this.onTabSwitch, this);
this.tabbox_.addEventListener("select",
                              this.onTabSwitchClosure_, true);

There is now a TabSelect event you can use (bug 322898).