Bug 343839

Comment 1 John Arthorne 2011-04-26 10:24:13 EDT

The theory is this might be related to Susan's recent change to support proper links in menus. Gosia to take an initial look but it might need some help from Susan. This will be a performance killer running on orionhub so we've got to do something for M7.

Comment 2 Susan McCourt 2011-04-26 11:51:53 EDT

(In reply to comment #1)
> The theory is this might be related to Susan's recent change to support proper
> links in menus. Gosia to take an initial look but it might need some help from
> Susan. This will be a performance killer running on orionhub so we've got to do
> something for M7.

I don't think this a new problem from a command framework point of view.  This is likely bug 338888.  When I wrote that bug I was more worried about dom bulk.  But the "new news" might be that the git code uses remote calls to compute the href links???  We went to some effort to ensure that things like validation rules get computed by the client.  If we are computing other necessary information in remote calls, then we would see this problem.

Comment 3 Susan McCourt 2011-04-26 12:06:31 EDT

Based on URL traffic, I think "Git Remote" is the command that has introduced the problem.  Is this the first remote href call added to the navigator?

getDefaultRemoteBranch called
gitClient.js (line 470)
GET http://localhost:8080/git/remote/file/D/web-ide.launch
GET http://localhost:8080/git/remote/file/D/web-ide.launch
GET http://localhost:8080/git/remote/origin/file/m/
GET http://localhost:8080/git/remote/origin/file/m/
GET http://localhost:8080/git/remote/origin/file/D/
GET http://localhost:8080/git/remote/origin/file/D/
GET http://localhost:8080/git/remote/origin/file/V/
GET http://localhost:8080/git/remote/origin/file/V/
GET http://localhost:8080/git/remote/origin/file/b/
GET http://localhost:8080/git/remote/origin/file/b/
GET http://localhost:8080/git/remote/origin/file/k/
GET http://localhost:8080/git/remote/origin/file/k/

I was planning to look at bug 338888 for M8 so that we could change the timing of the menu population, but I think we need a stopgap solution for now, such as moving git remote to the "more actions" menu or removing it from the navigator completely. 

Performance (bug 338888) aside, I think we are growing too many commands on the navigator rows.  (See also bug 343450).

Comment 4 Susan McCourt 2011-04-26 13:20:09 EDT

Gosia and I chatted about this. Here is the problem.

- commands can have an href callback, and we support the href callback
returning a promise.   Originally this was to support plug-ins being able to
calculate hrefs.
- now we have the git remote command, which is a slightly different flavor. 
The item in the navigator does not carry the git remote location info in it, so
the href callback has to do a GET just to get the URL that should be used to
view the remote.  This results in a GET on every visible item in the navigator.

There are some long term issues to consider as well, but it is not likely we
could get any of these implemented and tested for M7:

1 - if we really want git remote access from the navigator page, then in the long run I think GIT should adorn the JSON item with the git remotes information, rather than have the client code have to go back and request it. Up to now, the items contain the locations needed for navigator-based commands (like export, import, etc.)

2 - fixing bug 338888 would address *when* the computations happen.  The
discussion there is to only get the command info on hover or on menu open. 
This reduces the number of GETS because only items hovered or selected do the
computation, but it could cause some noticeable delays in hover or menu
operation. 

3 - it is possible, (but not common and not yet implemented), that a project
would be connected to multiple remotes.  So using the href command for a remote
is not a long term UI solution. Another possibility would be to include a list
of remotes in a submenu, but we would have the same response time problem here
if #1 was not implemented.  Or maybe we go to a remotes page that lists the
possible remotes.

4 - we have grown *a lot* of navigator commands in M7 and I'd like to do some
reorganization and grouping during M8.  However it'd be nice to do this from a
usability standpoint, not to work around performance problems.

All that said, Gosia and I discussed some workarounds for M7:
1 - make a placeholder page that does the GET for the actual URL.  This placeholder page is the href used in the command, and it loads the new page when the address is obtained.  
2 - we move the git remote command to the "more actions" menu so that the gets
are only done when the menu opens.  Neither of us likes solving a perf problem
with UI change, but this could be done.
3 - replace the href callback in the command with a normal callback that
computes the URL and then switches the page.  This ensures that the GET is only
done when the user actually clicks on the link.  The downside is that we lose
the link context menu behavior because the href is not really known in the
anchor link.  Plus the command ends up replicating stuff that should be done by the framework.  We could also decide to do this inside the framework with all hrefs.   We'd still have links and the click/ctrl-click behavior, but we'd lose the link context menu behavior since the href is not known until click.

Of all these possibilities, I like #1.  To me, it feels like the git remote
command is going to grow into a "list remotes" page anyway.  So this
placeholder page is kind of like a "list remotes" that figures out there is
only one and redirects to the info about that one remote.

Comment 5 Susan McCourt 2011-04-26 14:20:46 EDT

(In reply to comment #2)
> (In reply to comment #1)
> > The theory is this might be related to Susan's recent change to support proper
> > links in menus. Gosia to take an initial look but it might need some help from
> > Susan. This will be a performance killer running on orionhub so we've got to do
> > something for M7.
> 
> I don't think this a new problem from a command framework point of view.  This
> is likely bug 338888.  

I stand corrected.  It is bug 338888.  And command "links" have always exhibited this problem.  But links in menus never used to retrieve the href until onclick, and with my recent changes, the call happens up front now.   Fixing bug 338888 would mean simply that the call happened when the menu opened rather than up front, which is definitely better.

Comment 6 Susan McCourt 2011-04-26 14:27:23 EDT

I talked to Szymon.
For M7 we'll do #3.  
He is going to change his git remote command so that it computes the href on click.  This means git remote won't get a "real link" in the menu.

We think the right thing to do for M8 is:
- server side should include git remote locations in the items
- command framework shouldn't try to compute href links in menus until the menu opens (this is bug 338888).

Comment 7 Szymon Brandys 2011-04-28 07:15:44 EDT

(In reply to comment #6)
> I talked to Szymon.
> For M7 we'll do #3.
> He is going to change his git remote command so that it computes the href on
> click.  This means git remote won't get a "real link" in the menu.

I noticed yesterday that I can't fix it this way. The problem is that I can't open a new window or change the window location from within a plug-in.

> We think the right thing to do for M8 is:
> - server side should include git remote locations in the items

I changed the API, so the remote branch is now returned in the response.