946305 - Simplify BrowserToolbar listeners

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

With the new about:home we added a ton of callback listeners to the browser toolbar, most of which all call back to BrowserApp.

BrowserToolbar.OnActivateListener() { public void onActivate(); }
BrowserToolbar.OnCommitListener() { public void onCommit(); }
BrowserToolbar.OnDismissListener() { public void onDismiss(); }
BrowserToolbar.OnFilterListener() { public void onFilter(String searchText, AutocompleteHandler handler); }
BrowserToolbar.OnStartEditingListener() { public void onStartEditing(); }
BrowserToolbar.OnStopEditingListener() { public void onStopEditing(); }
Plus a keyEvent listener.

The lifecycle of these is confusing. We wind up taking different paths and running completely different code if you commit vs dismiss vs. stop editing. onEnterEditingMode actually calls BrowserToolbar.startEditing() which is cased to no-op if its already editing. etc. I have no idea what onActivate does. Its confusing... Also, none of the callbacks get any data sent to them which is surprising. I'd at least expect onCommit to receive whatever was committed.

I noticed this because in onStopEditing we call updateDoorhanger's now, which was done to reshow any doorhangers hidden while in editing mode. That code isn't aware of if a location change is coming currently, and so shows doorhangers even if we're about to load a new page. Rather than try and save state in all the possible exits from the doorhanger that could load a url, it makes more sense to me to try and reduce the paths out.

Also, I'm not sure why each one is its own interface. Do we have a use case for registering different listeners for onStart and onStop?

I'd like a system with fewer entry and exit points and a more clear flow through them. i.e. Something like:

BrowserToolbar.setListener(new BrowserToolbar.EditListener() {
  // Can we push onActivate/onStartEditing into one method?
  public void onStart();

  // Can we push onCommit/Dimsiss/StopEditing into one method that takes relevant data as parameters
  // enum Reason { TYPED, CANCELLED, CLICKED } ?
  public void onStop(Reason reason, String url);

  // Can we push both the keyEvent listener and the filter listener into one method?
  public void onEdit(KeyEvent event, String searchTerm, AutocompleteHandler handler);
});

Comment 1

[Lucas Rocha (:lucasr)]

Yeah, we started with a few listeners and things got a bit bloated since then. I'm adding this to the larger toolbar refactoring effort btw. 

(In reply to Wesley Johnston (:wesj) from comment #0)
> With the new about:home we added a ton of callback listeners to the browser
> toolbar, most of which all call back to BrowserApp.
> 
> BrowserToolbar.OnActivateListener() { public void onActivate(); }
> BrowserToolbar.OnCommitListener() { public void onCommit(); }
> BrowserToolbar.OnDismissListener() { public void onDismiss(); }
> BrowserToolbar.OnFilterListener() { public void onFilter(String searchText,
> AutocompleteHandler handler); }
> BrowserToolbar.OnStartEditingListener() { public void onStartEditing(); }
> BrowserToolbar.OnStopEditingListener() { public void onStopEditing(); }
> Plus a keyEvent listener.
> 
> The lifecycle of these is confusing. We wind up taking different paths and
> running completely different code if you commit vs dismiss vs. stop editing.
> onEnterEditingMode actually calls BrowserToolbar.startEditing() which is
> cased to no-op if its already editing. etc. I have no idea what onActivate
> does. Its confusing... Also, none of the callbacks get any data sent to them
> which is surprising. I'd at least expect onCommit to receive whatever was
> committed.
> 
> I noticed this because in onStopEditing we call updateDoorhanger's now,
> which was done to reshow any doorhangers hidden while in editing mode. That
> code isn't aware of if a location change is coming currently, and so shows
> doorhangers even if we're about to load a new page. Rather than try and save
> state in all the possible exits from the doorhanger that could load a url,
> it makes more sense to me to try and reduce the paths out.
> 
> Also, I'm not sure why each one is its own interface. Do we have a use case
> for registering different listeners for onStart and onStop?
> 
> I'd like a system with fewer entry and exit points and a more clear flow
> through them. i.e. Something like:
> 
> BrowserToolbar.setListener(new BrowserToolbar.EditListener() {
>   // Can we push onActivate/onStartEditing into one method?
>   public void onStart();
> 
>   // Can we push onCommit/Dimsiss/StopEditing into one method that takes
> relevant data as parameters
>   // enum Reason { TYPED, CANCELLED, CLICKED } ?
>   public void onStop(Reason reason, String url);
> 
>   // Can we push both the keyEvent listener and the filter listener into one
> method?
>   public void onEdit(KeyEvent event, String searchTerm, AutocompleteHandler
> handler);
> });

Agree with the general intent.

Comment 2

[:Margaret Leibovic]

If we haven't needed to refactor something in years, we don't need to refactor it.