Merge lp:~robertcarr/bamf/webapps into lp:bamf/0.4

[Robert Carr]

Add webapps support

Comments that I removed were fully integrated

>> 1308 +void
>> 1309 +bamf_application_set_application_type (BamfApplication *application, const gchar *type)

>> An enum for types would be better than using a string value.

What is the preferred way to keep the enum in sync over DBus?

>> 1584 +BamfView *
>> 1585 +bamf_application_get_focus_child (BamfApplication *application)
>> get_focused_child would be a better name imho

This method is perhaps a little confusing. It is not to return the focused child though, but the child which should be focused if this application is to be activated. It's used because only the webapps-context-daemon knows the order of tab activity.

>> Instead of this, it would be probably better to add another method to the class that can be implemented by each BamfApplication implementation.
>> Otherwise... Another possibility would be to define a timeout on bamf_view that defines the amount of time after that a view is considered closed.

I like the class method. Done

>> 1689 +
>> 1690 + bamf_unity_webapps_observer_new ();

I am initializing it and tracking it in bamf-matcher now.

>> This looks over complicated... also instead of checking for == TRUE/FALSE I'd prefer to just check running/!running
>> Also, isn't enough not to set the running state?

?

>> How much would cost at this point to rename BAMF_TAB to BAMF_BROWSER_TAB?
Can we do this later?

>> Also, the library is not currently testable in an easy way, but the daemon is (and all the types it handles) so I think we need tests for them.
-.- We have some coverage by the webapps autopilot tests? Is this enough for now?

[Jason Smith (jassmith)]

Testing confirms this is working. The size of the code review however makes doing a complete code review rather difficult. I have looked over the source and it generally looks sane but would like Robert to state a summary of the changes made by this branch as he sees them. Getting a feel for the general ideas and intent would help greatly.

review: Needs Information

[Robert Carr (robertcarr)]

The general intent of the branch is to add support for the Unity Webapps System in BAMF.

In the Webapps system there is a concept of a "Context" which is a running webapp identified by (name, domain) and each context may have "Interests" which represent a specific view of the application (i.e. browser tab)

In this branch they are mapped in Bamf to BamfUnityWebappsApplication, which is made up of BamfTab (BamfUnityWebappsTab) children.

Interaction with the matcher is done through the BamfUnityWebappsObserver type which watches the WebappsService and creates the appropriate BamfUnityWebappsApplications.

[Jason Smith (jassmith)]

I have gone over the review again. As for some of the comments made in the original merge prop, no new code should be added to this. It is already to big. So do the renaming in another merge later.

+1

review: Approve

[Unity Merger (unity-merger)]

No commit message specified.

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-bamf/92/console reported an error when processing this lp:~robertcarr/bamf/webapps branch.
Not merging it.

[Marco Trevisan (Treviño) (3v1n0)]

Thanks for your fixes, they all looks fine to me.

>> 1308 +void
>> 1309 +bamf_application_set_application_type (BamfApplication *application, const gchar *type)

>> An enum for types would be better than using a string value.

> What is the preferred way to keep the enum in sync over DBus?

Well, probably not on DBUs, but on the daemon side it would be better a more type-safe and defined value than just a string.

>> get_focused_child would be a better name imho

> This method is perhaps a little confusing. It is not to return the focused child though,

Mh, so maybe getting a proper name could be better :)

I know it's annoying... But I still see some tabs around in the code. It would be nice to get these removed before merging.

>> This looks over complicated... also instead of checking for == TRUE/FALSE I'd prefer to just check running/!running
>> Also, isn't enough not to set the running state?

> ?

I meant about this, sorry:

1454 + if (!((close_when_empty == FALSE) && running == FALSE))
1455 + bamf_view_set_user_visible (BAMF_VIEW (self), visible);
1456 + if ((running == TRUE) || close_when_empty)
1457 + bamf_view_set_running (BAMF_VIEW (self), running);

The first check for example could be just (!(!close_when_empty && !running)), even if I'd prefer the reversed form.
Also the bamf_view_set_running probably is enough, isn't it?

>> Also, the library is not currently testable in an easy way, but the daemon is (and all the types it handles) so I think we need tests for them.
> -.- We have some coverage by the webapps autopilot tests? Is this enough for now?

Well, AP could look too "far" from the actual implementation and it would be nice to get some new features tested here as well, however I can go with this, but the last word is up to the distro, btw.

[Marco Trevisan (Treviño) (3v1n0)]

Ok, fine now.
Thanks! ;)

review: Approve

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-bamf/93/console reported an error when processing this lp:~robertcarr/bamf/webapps branch.
Not merging it.

[Sebastien Bacher (seb128)]

the merger failed on a "No package 'libunity_webapps-0.2' found", I've added a Build-Depends on libunity-webapps-dev to the packaging branch, let's retry the merge

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-bamf/94/console reported an error when processing this lp:~robertcarr/bamf/webapps branch.
Not merging it.

[lp:~robertcarr/bamf/webapps]

500. By Robert Carr on 2012-08-22

Don't unref the UnityWebappsService if we never create it