Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 352083 - ProgressListener.completed() not always called
Summary: ProgressListener.completed() not always called
Status: RESOLVED FIXED
Alias: None
Product: RAP
Classification: RT
Component: RWT (show other bugs)
Version: 1.4   Edit
Hardware: All All
: P2 normal (vote)
Target Milestone: 1.5 RC1   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard: sr141
Keywords:
: 379349 (view as bug list)
Depends on: 327887
Blocks: 374244
  Show dependency tree
 
Reported: 2011-07-14 09:20 EDT by Dominik G. CLA
Modified: 2012-05-13 06:06 EDT (History)
5 users (show)

See Also:


Attachments
Screenshot showing the load bug. (346.37 KB, image/jpeg)
2011-07-14 09:21 EDT, Dominik G. CLA
no flags Details
Screenshot showing the reset bug. (117.94 KB, image/jpeg)
2011-07-14 09:21 EDT, Dominik G. CLA
no flags Details
Screenshot showing the load bug with the IE 9.0. (374.82 KB, image/jpeg)
2011-07-14 11:18 EDT, Dominik G. CLA
no flags Details
Patch for v1.4_Maintenance branch (3.64 KB, patch)
2011-07-15 03:26 EDT, Ivan Furnadjiev CLA
no flags Details | Diff
Patch for v1.4_Maintenance branch (4.58 KB, patch)
2011-07-22 04:23 EDT, Ivan Furnadjiev CLA
ivan: review?
rsternberg: review+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik G. CLA 2011-07-14 09:20:21 EDT
Build: RAP 1.4 (with integrated fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=349671),
       RAP Nightly Build (rap-runtime-1.5.0-N-20110713-2127.zip)

Bug 1:
If I use multiple browser widgets at the same time the completed() event is not received by all browser widgets (see screenshot: bug_load.jpg).

While I have created this snippet to reproduce this behaviour I detected another strange behaviour.

Bug 2:
If I call browser.setUrl("") to clear all the browser widgets, some of them receive a completed() event.
I think this event should be fired for all browser widgets or none (see screenshot: bug_reset.jpg). 

  
Here is the snippet to reproduce:


public class ProgressListenerCompletedBug2 implements IEntryPoint {

	public int createUI() {
		Display display = new Display();
		Shell shell = new Shell(display, SWT.NO_TRIM);
		shell.setMaximized(true);
		createContent(shell);
		shell.layout();
		shell.open();

		while (!shell.isDisposed()) {
			if (!display.readAndDispatch()) {
				display.sleep();
			}
		}
		return 0;
	}

	private void createContent(Shell shell) {
		shell.setLayout(new GridLayout(4, false));

		Button buttonReset = new Button(shell, SWT.PUSH);
		buttonReset.setLayoutData(new GridData(SWT.CENTER, SWT.CENTER, true, false, 4, 1));
		buttonReset.setText("Reset");

		Button buttonLoad = new Button(shell, SWT.PUSH);
		buttonLoad.setLayoutData(new GridData(SWT.CENTER, SWT.CENTER, true, false, 4, 1));
		buttonLoad.setText("Load");

		String url = "http://www.eclipse.org";
		int count = 26;
		final List<ResultHelper> browserList = new ArrayList<ResultHelper>();
		for (int i = 0; i < count; i++) {
			browserList.add(createBrowserArea(shell, url));
		}

		buttonReset.addSelectionListener(new SelectionAdapter() {

			@Override
			public void widgetSelected(SelectionEvent e) {
				for (ResultHelper helper : browserList) {
					showEmptyMessage(helper.text);
					helper.browser.setUrl("");
				}
			}

		});
		buttonLoad.addSelectionListener(new SelectionAdapter() {

			@Override
			public void widgetSelected(SelectionEvent e) {
				for (final ResultHelper helper : browserList) {
					showEmptyMessage(helper.text);
					helper.browser.setUrl(helper.url);
					showLoadingMessage(helper.text);
				}
			}

		});
	}

	private ResultHelper createBrowserArea(Composite parent, String url) {
		final Browser browser = new Browser(parent, SWT.BORDER);
		browser.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true));

		final Text text = new Text(parent, SWT.SINGLE | SWT.CENTER | SWT.BORDER);
		text.setLayoutData(GridDataFactory.swtDefaults()
				.align(SWT.FILL, SWT.FILL).hint(200, 15).create());
		showEmptyMessage(text);

		browser.addProgressListener(new ProgressListener() {

			public void completed(ProgressEvent event) {
				showCompletedMessage(text);
			}

			public void changed(ProgressEvent event) {
				// nothing
			}

		});

		return new ResultHelper(browser, text, url);
	}

	private void showEmptyMessage(Text text) {
		text.setBackground(text.getDisplay().getSystemColor(SWT.COLOR_WHITE));
		text.setText("");
	}

	private void showLoadingMessage(Text text) {
		text.setBackground(text.getDisplay().getSystemColor(SWT.COLOR_YELLOW));
		text.setText("Loading URL ...");
	}

	private void showCompletedMessage(Text text) {
		text.setBackground(text.getDisplay().getSystemColor(SWT.COLOR_GREEN));
		text.setText("Completed event detected!");
	}

	private static class ResultHelper {

		Browser browser;
		Text text;
		String url;

		public ResultHelper(Browser browser, Text text, String url) {
			super();
			this.browser = browser;
			this.text = text;
			this.url = url;
		}

	}

}


Results:
- Most of the time the completed() event is called on every browser widget for each "load" action. From time to time some browser widgets were not informed.
- On the "reset" action, some widgets receive a completed() event while the majority doesn't.  

Expected Result:
- ProgressListener.completed() will always be called if the page was loaded.
- If we reset the browser URL the behaviour should be the same for all widgets. 


Please add a fix also for the comming RAP 1.4SR1.
(Prerequisite: bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=349671 will also be added for RAP 1.4SR1).
Comment 1 Dominik G. CLA 2011-07-14 09:21:31 EDT
Created attachment 199659 [details]
Screenshot showing the load bug.
Comment 2 Dominik G. CLA 2011-07-14 09:21:58 EDT
Created attachment 199660 [details]
Screenshot showing the reset bug.
Comment 3 Ivan Furnadjiev CLA 2011-07-14 09:43:19 EDT
Dominik, did you test it in all browsers ( IE, Firefox, Chrome, Safari, Opera )? Is this happen in all browsers? Could you please set a different URL on every browser? Is it still reproducible?
Comment 4 Dominik G. CLA 2011-07-14 11:14:55 EDT
(In reply to comment #3)
> Dominik, did you test it in all browsers ( IE, Firefox, Chrome, Safari, Opera)? 
I only testes it with Firefox 3.6.18, Firefox 5.0 and IE 9.0.

> Is this happen in all browsers? 
> Could you please set a different URL on every browser? Is it still reproducible?
This bug occured in our application where all browser widgets display a different URL,
so I created this snippet to reproduce. 
First I wanted to use different URLs, but I noticed that this bug also occured with the same URL.
I have made a concrete test with different URLs at this moment 
and YES this bug is still reproducable with the three mentioned browsers.


At the moment I realised that the Firefox does a much better job than the IE.
With the Firefox I need 2-5 "reset/load" steps to reproduce at least one missing completed() event.
With the IE more than 50% of the browser widgets do not receive a completed() event at each "load" step.
(If I use the same URL the missing completed() event rate is greater than 85%)
 
Can you reproduce this behaviour on your site?
Comment 5 Dominik G. CLA 2011-07-14 11:18:03 EDT
Created attachment 199673 [details]
Screenshot showing the load bug with the IE 9.0.
Comment 6 Ivan Furnadjiev CLA 2011-07-14 11:54:07 EDT
(In reply to comment #4)
> Can you reproduce this behaviour on your site?
Yes... I can reproduce it and I can see where is the problem. The progress event ( as all other events in RAP ) are sent from client to the server as request parameter like:
org.eclipse.swt.events.progressCompleted=w55
where "w55" is the widget (Browser) id. In case of multiple Browser widgets that fired complete event almost in the same time (in the time slot of request execution) this request parameter is overrided by every new complete event. At the end, some complete events are lost, only last one reach the server with the next request. There is an open bug for this issue - bug 327887. I think that this issue will be fixed with the implementation of the new client-server protocol - bug 311355.
Comment 7 Dominik G. CLA 2011-07-14 12:42:34 EDT
(In reply to comment #6)
Thank you for the background information.
So I try to find a workaround that doesn't depend on the completed() event for our usecase.

Nevertheless do you think the bug 349671 will be inside RAP 1.4SR1. If I'm not wrong, the patch was not added to the maintenance branch of 1.4 until now.
We have other usecases where we have only one browser widget, 
so the problem with many events of the same type, almost within the same time, does not matter here.
Comment 8 Ivan Furnadjiev CLA 2011-07-14 13:19:17 EDT
(In reply to comment #7)
> Nevertheless do you think the bug 349671 will be inside RAP 1.4SR1.
Yes... the patch will land in v1.4_Maintenance branch soon.
Comment 9 Ivan Furnadjiev CLA 2011-07-15 03:26:13 EDT
Created attachment 199727 [details]
Patch for v1.4_Maintenance branch

This patch replaces req.addEvent with req.addParameter to be possible to handle sending multiple events from the same type at once. Changed BrowserLCA, LCA test and js tests accordingly.
Comment 10 Ivan Furnadjiev CLA 2011-07-15 03:33:50 EDT
For CVS HEAD I'm still thinking that this issue should be fixed in a generic way with the new client-server protocol – bug 311355
Comment 11 Dominik G. CLA 2011-07-15 09:33:08 EDT
(In reply to comment #9)
The patch passed all my test cases!
Many thanks for the quick fix.

You saved my weekend!
Comment 12 Ivan Furnadjiev CLA 2011-07-22 04:23:29 EDT
Created attachment 200159 [details]
Patch for v1.4_Maintenance branch

Due to overlapping changes with bug 349671 this patch includes both changes.
Comment 13 Ivan Furnadjiev CLA 2011-07-26 03:53:39 EDT
Applied patch to v1.4_Maintenance branch. In HEAD, this issue will be fixed in a generic way with bug 327887.
Comment 14 Tim Buschtoens CLA 2011-08-25 08:45:34 EDT
Commited patch to v14_Tree_Table_Merge branch.
Comment 15 Werner Hihn CLA 2012-03-20 06:07:00 EDT
Ivan, Tim, any plans if and when this will be fixed for RAP 1.5?
Comment 16 Tim Buschtoens CLA 2012-03-20 06:26:25 EDT
Since 1.5 won't have the client-to-server protocol implementation i think we should do it.
Comment 17 Werner Hihn CLA 2012-03-20 07:07:52 EDT
Great to hear! Thx
Comment 18 Christian Hager CLA 2012-05-08 17:45:24 EDT
Can you give any hint when this will be fixed in RAP 1.5?
Comment 19 Ivan Furnadjiev CLA 2012-05-09 04:18:38 EDT
(In reply to comment #16)
> Since 1.5 won't have the client-to-server protocol implementation i think we
> should do it.
Adjusted the attached patch to current CVS HEAD and applied it.
Comment 20 Ivan Furnadjiev CLA 2012-05-13 06:06:06 EDT
*** Bug 379349 has been marked as a duplicate of this bug. ***